Skip to content
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

Implement XDG Base Directory specification #405

Merged
merged 7 commits into from
Jun 8, 2021

Conversation

JustAnotherArchivist
Copy link
Contributor

ia will attempt the following paths in this order, using the first existing file:

  • ${XDG_CONFIG_HOME}/internetarchive/ia.ini (with XDG_CONFIG_HOME defaulting to ${HOME}/.config if not set or invalid)
  • ~/.config/ia.ini
  • ~/.ia

When none of these paths exist on creating a config file (ia configure), the first one is used and directories are created as necessary.

Closes #400

@JustAnotherArchivist
Copy link
Contributor Author

A note on os.environ['HOME'] vs os.expanduser('~'): the XDG specification requires using $HOME. os.expanduser falls back to using the password database or the root directory if $HOME doesn't exist and is therefore not fit for purpose. In practice, this shouldn't matter as POSIX requires the presence of that environment variable, but I feel like it was worth mentioning anyway.

@JustAnotherArchivist JustAnotherArchivist force-pushed the config-xdg branch 2 times, most recently from 96762dc to 97c4605 Compare May 5, 2021 02:13
@JustAnotherArchivist
Copy link
Contributor Author

For some reason, $HOME apparently isn't set in Travis CI. Lovely...
Not sure what the reason for that is. Smells like a bug on Travis's side.

I've successfully tested this code locally with a POSIX-compliant environment in all scenarios I could think of.

@JustAnotherArchivist
Copy link
Contributor Author

JustAnotherArchivist commented May 5, 2021

Rethinking this, I suppose ia should also work on Windows, where $HOME doesn't exist as far as I know. Perhaps using ~ is better after all; on any properly POSIX-compliant system, it will still behave according to the specification since expanduser uses $HOME if set. This would also work around the Travis CI bug. @jjjake, any thoughts on this?

`ia` will attempt the following paths in this order, using the first existing file:

- `${XDG_CONFIG_HOME}/internetarchive/ia.ini` (with `XDG_CONFIG_HOME` defaulting to `~/.config` if not set or invalid; see code comment regarding `~` vs `$HOME`)
- `~/.config/ia.ini`
- `~/.ia`

When none of these paths exist on creating a config file (`ia configure`), the first one is used and directories are created as necessary.
@JustAnotherArchivist
Copy link
Contributor Author

I made that change from $HOME to ~ as I can't really think of a good reason not to do that. The specification doesn't say anything about what should happen if $HOME isn't present, presumably because it assumes a POSIX-compliant system (without mentioning it). And on such systems, the new code will still behave according to the specification because Python's expansion uses $HOME if present on Unix and only falls back to the password database if not; on Windows, $HOME is useless anyway.

I would still like to figure out why the build failed though. I wrote to Travis CI, and they confirmed that $HOME works correctly in an independent test build. I'll probably create a separate PR to play with that.

Also, I still want to add some tests here to ensure that a #399 situation can't happen again. So this isn't ready for merging yet.

@JustAnotherArchivist
Copy link
Contributor Author

Tests are succeeding except on Python 2.7 because tempfile.TemporaryDirectory doesn't exist there. I could try to reimplement that, but Py2 is EOL and ia is already broken on it (#385), so I'm not sure that's worth the effort. @jjjake, please advise.

@JustAnotherArchivist
Copy link
Contributor Author

I took a quick look at the implementation of tempfile.TemporaryDirectory, and it's more complex than I thought. If dropping Py2 support is not yet an option, skipping these tests on Py2 would be another possibility.

@jjjake
Copy link
Owner

jjjake commented May 10, 2021

I understand PY2 is EOL, but it's still widely used in reality and something I would like to support as long as we're able to. Any changes made in support of XDG_CONFIG_HOME should not break ia configure, etc., in PY2.

@JustAnotherArchivist
Copy link
Contributor Author

Alright, I understand. To be clear, it's only the tests that are held back by this; the actual code should not be Py3-specific in any way.

I'll try to write a minimal crutch for Py2 that emulates the key part of TemporaryDirectory. I have no desire to reimplement its complex error handling though – a lot of things in that area changed between Py2 and Py3 and I haven't written code for Py2 in years... But it should be good enough to get the test suite running on CI at least.

This simplifies testing of the latter method greatly as there is no need to mock get_auth_config. This separation also makes much more sense from the naming side of things.
@JustAnotherArchivist JustAnotherArchivist force-pushed the config-xdg branch 2 times, most recently from 67a069c to 8857f30 Compare June 6, 2021 23:14
@JustAnotherArchivist
Copy link
Contributor Author

I realised today that write_config_file was broken on Python 2 for the same reason as the tests were previously (lack of exist_ok kwarg), but because there were no tests for that method at all, it didn't trip anything. I refactored that code slightly to decouple the IA login from the config file writing and added tests for all of that as well.

This should now be ready for review and merging. :-)

@jjjake jjjake merged commit 295150e into jjjake:master Jun 8, 2021
@jjjake
Copy link
Owner

jjjake commented Jun 8, 2021

Thanks again, @JustAnotherArchivist!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XDG_CONFIG_HOME is not respected
2 participants