-
Notifications
You must be signed in to change notification settings - Fork 502
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat!: switch to adrg/xdg library over go-xdg #3535
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this change. We would be going from the home-grown BDS support to something that has not reached version 1, but aren't using the features that I believe make adrg/xdr interesting:
configFilePath, err := xdg.ConfigFile("appname/config.yaml")
It also does not provide any fallback behaviour for people using the current behaviour if they are on macOS.
I would almost rather see a PR against twpayne/go-xdg to add a platform support mode to that library and perhaps some of these configuration file locators, then updating chezmoi to use this.
Ideally, because chezmoi is starting from a "dirty" state, it should do the following on macOS (and something equivalent on Windows):
- If
--config
is provided, use the resolved absolute path. - If it is not provided, locate the configuration file:
- Look for configuration in the "linux" XDG location; if it exists there, use that configuration file.
- Look for configuration in the macOS "XDG" location. If it exists there, use that configuration file, or create the configuration there.
For configuration, this would require documentation updates and would be more than a little confusing initially. I would personally reconsider the approach to do:
- Update twpayne/go-xdg to support ways of getting the "canonical" XDG BDS configuration values and the "platform-equivalent" configuration values. Non-Linux systems should be able to do both. Perhaps even with Linux, specifying falling back to
~/.config
even if$XDG_*
are set might be useful — as I noted, chezmoi does not do well if$XDG_*
values are outside of$HOME
. - Update chezmoi to put the
httpcache
directory / file (currently colocated with the.boltdb
) in a proper cache directory. Because it is currently colocated with.boltdb
and the config file, it may be desirable to have it put into a hash directory within that cache directory. - Update chezmoi to use platform-native cache directories. Ideally, this should migrate non-native (XDG) cache to native cache so that large files are not re-requested because they are not in the expected location.
The conservative form of this would be to check whether the files are already in the XDG location; if they are, don't do anything. If they aren't, you can use platform config files.
|
||
logger := zerolog.Nop() | ||
|
||
c := &Config{ | ||
ConfigFile: newConfigFile(bds), | ||
ConfigFile: newConfigFile(xdgCacheHome), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be xdgConfigHome
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bds
argument that that function took prior was only used to determine the cache directory, and as such I simply passed the required cache path directly. I realise that this looks pretty confusing, though.
// Check for XDG Base Directory Specification data directories first. | ||
for _, dataDir := range bds.DataDirs { | ||
for _, dataDir := range c.xdgDataDirs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be very cautious about using xdgDataDirs
, as it points to directories outside of $HOME
and does not clearly include $XDG_DATA_HOME
in the documentation for adrg/xdr. Chezmoi is a personal dotfiles manager and should not be accessing anything outside of $HOME
or maybe /tmp
(/private/tmp
on macOS).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, xdg.DataDirs
does not include xdg.DataHome
, but that is why i'm using the xdgDataDirs
field to also include xdg.DataHome
and retain the behaviour that was present in go-xdg
.
Given what you said, I'm happy to reconsider the approach and instead see what I can do with the upstream |
Note that Instead, chezmoi should be extended with functions like |
Concerns were brought up in #3528 regarding the use of Unix-like XDGBDS paths on systems such as Windows and macOS, which use alternative schemes such as
%APPDATA%
etc. and~/Library/Preferences
etc.As requested in #2673 (reply in thread), this PR aims to address those concerns by switching to a library known to provide these alternative paths on such systems with their own scheme, rather than the in-house solution
go-xdg
which falls back exclusively onto the XDGBDS defaults while ignoring alternative schemes.WIP: A migration path will probably need to be considered for existing users making use of the XDG defaults on systems such as Windows. Also, tests need to be adapted for Windows and macOS.