-
Notifications
You must be signed in to change notification settings - Fork 22
Use XDG base directories, if available #110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Hi @tapeinosyne, thank you for your contribution! I think this is a good improvement.
--home
I am not too clear on the purpose of Sheldon's --home flag, which is why this code still aborts if XDG is set but a home folder cannot be found. Does home serve a purpose beyond providing a prefix for the default root? In which cases would a user pass --home rather than --root?
Right now the home directory is used for the following things:
- Figuring out the default root directory.
- Replacing the home directory with a tilde (
~
) in the UI output. - Expanding tilde's in paths in the config file.
--home
would only be used if you were running on some unknown Linux where the home
package could not figure out the directory. This is highly unlikely.
XDG Base Directory
In the docs it specifies the following:
-
XDG_CONFIG_HOME
Where user-specific configurations should be written (analogous to /etc). Should default to $HOME/.config. -
XDG_CACHE_HOME
Where user-specific non-essential (cached) data should be written (analogous to /var/cache). Should default to $HOME/.cache.
It seems to me that it might make more sense that if we used the cache directory as well?
The current default folder structure is like the following.
$HOME/.sheldon # root / cache_dir?
plugins.toml # config_file
plugins.lock # lock_file
repos/ # clone_dir
downloads/ # download_dir
I think that plugins.lock
, repos/
and downloads/
are more like cache information. So perhaps we should keep those in the the $XDG_CACHE_HOME/sheldon
directory.
If XDG_CONFIG_HOME
then we could have a folder structure like this.
$XDG_CONFIG_HOME/sheldon # root
plugins.toml # config_file
$XDG_CACHE_HOME/sheldon # cache_dir?
plugins.lock # lock_file
repos/ # clone_dir
downloads/ # download_dir
What do you think?
It certainly would.
I myself feel a degree of uncertainty over the difference between "essential"
The Debian Wiki entry might help understand my reasoning. Consider:
The One last thingThere is another small decision to make: I reckon it should be reasonable to assume that, if any |
Yeah I agree. I'm happy with your proposed directory structure, although it might be simpler to just put Implementation details
Tests not passingYou should rebase your branch on the latest master, there was some weird things with linking when cross compiling which are resolved now on the latest master. |
Yes, I thought about it as well. Even though using
Agreeable. Let's.
It might be best to lock
That would be a breaking change, correct? It's hard to judge how many people might be using
Absolutely. The amended commit will be rebased as a matter of course. |
The mutex is also used during any config edit commands (
Yes I am aware, I was thinking we would have to bump to 0.6.0 for these changes anyway because we are changing logic based on |
1fde240
to
0d039b2
Compare
The commit for XDG compliance has been amended to follow the revised folder structure. (With apologies for the I have left out the removal of |
XDG directory support is now covered by integration tests rather than unit tests. (The relevant commit can be squashed/rebased as you prefer prior to merging.) I have also taken the opportunity for a minor refactor, which will be needed in some fashion come the deprecation of |
XDG_CONFIG_HOME
, if availableThere 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 integration tests look great! Thanks for the hard work. Just one more small comment, but otherwise I think we good to go.
7147cb0
to
c6acaf8
Compare
… in compliance with the [XDG Base Directory specification]. With this commit, Sheldon will check for the following ENV variables: - `XDG_CONFIG_HOME`, `XDG_CONFIG_DIRS` - `XDG_DATA_HOME`, `XDG_DATA_DIRS` - `XDG_CACHE_HOME` If any such variable is defined, Sheldon will adopt the XDG folder structure and use standard defaults for undefined ones, like so: ``` $XDG_CONFIG_HOME/sheldon # XDG_CONFIG_HOME defaults to `~/.config` plugins.toml # config_file $XDG_DATA_HOME/sheldon # XDG_DATA_HOME defaults to `~/.local/share` repos/ # clone_dir downloads/ # download_dir plugins.lock # lock_file ``` XDG support is a breaking change for Sheldon users with defined `XDG_*` variables, as no effort is made to respect the folder structure of pre-existing installations: Sheldon will simply look for configuration and data in XDG folders, ignoring existing files. [XDG Base Directory specification]: https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html Co-authored-by: Ross MacArthur <ross@macarthur.io>
Hmmm, unfortunately because Sheldon's output is not ordered, the test case doesn't produce consistent output and thats why its failing on some builds. |
I suppose we could test only one of |
and refactor integration tests to better support alternative directory structures. Configuration and data are now separate, with `root` functioning mostly as a shorthand for Sheldon's default single-folder layout, `~/.sheldon`. Test cases also use a dedicated `home` to better reflect real-work usage.
… to avoid stderr ordering issues. The directory structure test case is now split into two separate phases: `downloads` only followed by `downloads`+ `repos`.
The directory structure for |
Great work! Thanks @tapeinosyne 🙌 |
Excellent. I shall move on to the deprecation of root, then. |
This PR adds basic support for XDG base directories; specifically, it makes
XDG_CONFIG_HOME/sheldon
the default root folder for Sheldon if$XDG_CONFIG_HOME
is defined. If desired, the PR could be easily updated to make appropriate use ofXDG_DATA
as well asXDG_CONFIG
.There is no change in functionality: it's just a simple way to help users keep their home folders tidy.
Two notes on the patch:
XDG_CONFIG_HOME
is simply ignored if it is not valid Unicode. Do we want to alert the user and abort instead?--home
flag, which is why this code still aborts ifXDG
is set but a home folder cannot be found. Doeshome
serve a purpose beyond providing a prefix for the default root? In which cases would a user pass--home
rather than--root
?