Fix configuring `astropy` cache location by eerovaher · Pull Request #17514 · astropy/astropy

eerovaher · 2024-12-06T17:39:26Z

Description

657c91a (from #16997) introduced a bug to customizing the astropy cache directory. The XDG_CACHE_HOME environment variable has no effect, instead XDG_CONFIG_HOME controls both configuration and cache directories.

The first commit adds a regression test to reveal the bug, the second fixes it.

PS https://www.astropy.org/team does not document who the maintainers for config are.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-12-06T17:39:54Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

mhvk

Looks good to me, thanks @eerovaher!

saimn · 2024-12-06T19:55:47Z

astropy/config/paths.py

    if (
-        (xdg_config_home := os.getenv("XDG_CONFIG_HOME")) is not None
-        and (xch := Path(xdg_config_home)).exists()
+        (xdg_dir := os.getenv(f"XDG_{fallback.upper()}_HOME")) is not None


Might be useful to specify above that fallback can take a limited number of values.

yes. We could also expose the environment variable directly as a separate argument, since it's a private function, there's no pressure to preserve its API.

Might be useful to specify above that fallback can take a limited number of values.

A good way to do that would be to change the type annotation of the fallback parameter from str to Literal["cache", "config"], but it would also be a good idea to update the annotation of the cls parameter from type to type[_SetTempPath] and there is more room for improvement elsewhere in this file. My preference would be to only make minimal required changes here to fix the bug and to open a separate pull request for the annotations.

Sounds like a plan to me. Final question before I merge: do you want to do the follow up yourself too ? (either way is fine)

I wouldn't mind updating the annotations myself.

alright then

eerovaher · 2024-12-09T15:29:03Z

I've figured out why the double test CI job failed, but I don't have a good solution how to get it to work. The double test job uses the astropy test runner, whereas the other jobs run pytest directly:

astropy/tox.ini

Line 125 in 2e7cf84

    
           double: python -c 'import sys; from astropy import test; test(); sys.exit(test())'

The astropy test runner configures the cache directory using set_temp_cache():

astropy/astropy/tests/runner.py

Line 253 in 2e7cf84

with set_temp_cache(astropy_cache, delete=True):

set_temp_cache() is supposed to have priority over environment variables, which fully explains why changing the environment variables in the new test has no effect in that one CI job.

The best solution would be to skip the new test if and only if the tests are run using the astropy test runner, but I don't know how to do that. A simple solution would be to use set_temp_cache() and set_temp_config() in the test, but I don't like the idea of adding these extra steps to the test just because of the astropy test runner.

neutrinoceros · 2024-12-09T15:51:41Z

A simple solution would be to use set_temp_cache() and set_temp_config() in the test, but I don't like the idea of adding these extra steps to the test just because of the astropy test runner.

One more data point in favor of killing the test runner, but as long as we agree to live with it, this might be the only way out.

pllim

Good catch. Thanks!

Original config designer was probably mdboom who left some time ago. 🙈

pllim · 2024-12-09T20:21:06Z

Your tests seem to fail in the "double" run, so that needs to be fixed before this can be merged: https://github.com/astropy/astropy/actions/runs/12203451342/job/34046435787?pr=17514

eerovaher · 2024-12-09T21:58:06Z

Your tests seem to fail in the "double" run, so that needs to be fixed before this can be merged: https://github.com/astropy/astropy/actions/runs/12203451342/job/34046435787?pr=17514

I find it quite disturbing that you have approved this pull request when it is clear that you have not read the discussion at all.

mhvk · 2024-12-09T22:12:22Z

Like many (certainly like me), @pllim probably looks at the actual changes and approves those, and then realizes a test is broken. That that was discussed further above just goes to show that the github way of organizing things is far from optimal -- reading longer discussions is extremely tedious since there is no organization, no way to resolve pieces of the main thread, etc.

Anyway, @pllim, know we know you are the one that keeps this ship going! Thanks!!

pllim · 2024-12-09T23:02:26Z

Yes, indeed I jumped to the diff first. I do not have time to always read all the discussions, especially since now I am trying to play catch up after being away. But that is neither here nor there. I should probably dismiss my approval for now. Thanks, all.

Failing CI

pllim · 2024-12-09T23:09:19Z

Re: double trouble -- A workaround is to set a IS_DOUBLE for the "double" job and have a test skip on that condition. Maybe here:

astropy/tox.ini

Line 15 in 3ca1a10

setenv =

Or maybe like this:

https://github-actions-workflows.openastronomy.org/en/stable/tox.html#setenv

Not pretty but it would disappear when we remove the test runner.

neutrinoceros · 2024-12-11T15:21:33Z

I just merged #17529 and set its backport to auto-merge.

It should be possible to configure the `astropy` configuration and cache directories with the `XDG_CONFIG_HOME` and `XDG_CACHE_HOME` environment variables, but the new test reveals that the latter has no effect.

657c91a (MNT: Run PTH flake test in prep for supporting pathlib (config)) introduced a bug which caused the `XDG_CACHE_HOME` environment variable to be ignored and `XDG_CONFIG_HOME` to control both configuration and cache directories (if set). The correct behaviour has been restored, but removing any cache files from unintended locations is not attempted.

neutrinoceros · 2024-12-11T17:08:27Z

Approving now but giving time for @eerovaher to answer the one hanging thread before I merge

…514-on-v7.0.x Backport PR #17514 on branch v7.0.x (Fix configuring `astropy` cache location)

eerovaher added config Bug backport-v7.0.x labels Dec 6, 2024

eerovaher added this to the v7.0.1 milestone Dec 6, 2024

eerovaher requested review from neutrinoceros and pllim December 6, 2024 17:39

mhvk approved these changes Dec 6, 2024

View reviewed changes

saimn mentioned this pull request Dec 6, 2024

Let user define cache location #17507

Open

saimn reviewed Dec 6, 2024

View reviewed changes

pllim previously approved these changes Dec 9, 2024

View reviewed changes

neutrinoceros mentioned this pull request Dec 10, 2024

RFC: deduplicate implementations for get_config_dir and get_cache_dir #17188

Merged

1 task

eerovaher mentioned this pull request Dec 10, 2024

Change setting up temporary directories in astropy test runner #17529

Merged

1 task

eerovaher added 2 commits December 11, 2024 16:49

Add tests for XDG environment variables

4da0756

It should be possible to configure the `astropy` configuration and cache directories with the `XDG_CONFIG_HOME` and `XDG_CACHE_HOME` environment variables, but the new test reveals that the latter has no effect.

eerovaher force-pushed the read-xdg_cache_home branch from 56ddade to ce43637 Compare December 11, 2024 15:54

neutrinoceros approved these changes Dec 11, 2024

View reviewed changes

neutrinoceros merged commit 412bc56 into astropy:main Dec 12, 2024

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Dec 12, 2024

Backport PR astropy#17514: Fix configuring astropy cache location

09d3964

meeseeksmachine mentioned this pull request Dec 12, 2024

Backport PR #17514 on branch v7.0.x (Fix configuring astropy cache location) #17537

Merged

eerovaher deleted the read-xdg_cache_home branch December 12, 2024 13:45

pllim added a commit that referenced this pull request Dec 12, 2024

Merge pull request #17537 from meeseeksmachine/auto-backport-of-pr-17…

23ad50d

…514-on-v7.0.x Backport PR #17514 on branch v7.0.x (Fix configuring `astropy` cache location)

Uh oh!

Conversation

eerovaher commented Dec 6, 2024 • edited by pllim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions bot commented Dec 6, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

saimn Dec 6, 2024

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Dec 7, 2024

Choose a reason for hiding this comment

Uh oh!

eerovaher Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

eerovaher Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

eerovaher commented Dec 9, 2024

Uh oh!

neutrinoceros commented Dec 9, 2024

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Dec 9, 2024

Uh oh!

eerovaher commented Dec 9, 2024

Uh oh!

mhvk commented Dec 9, 2024

Uh oh!

pllim commented Dec 9, 2024

Uh oh!

pllim commented Dec 9, 2024

Uh oh!

neutrinoceros commented Dec 11, 2024

Uh oh!

neutrinoceros commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eerovaher commented Dec 6, 2024 •

edited by pllim

Loading

eerovaher Dec 11, 2024 •

edited

Loading

neutrinoceros commented Dec 11, 2024 •

edited

Loading