-
Notifications
You must be signed in to change notification settings - Fork 1k
tests/functional: assert R_OK/W_OK for XDG dirs #16322
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
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
03667a3
tests/functional: assert R_OK/W_OK for XDG dirs
woodruffw b8959cb
tests/functional: assert R_OK/W_OK for XDG dirs
woodruffw 76de855
Dockerfile: remove XDG settings
woodruffw 10c0a90
tests/functional: ensure_exists for platformdirs
woodruffw ad88a62
tests/functional: lintage
woodruffw 109c09e
config: ensure XDG dirs
woodruffw a15df01
requirements: make platformdirs a direct dep
woodruffw b85eb2b
config: lintage
woodruffw 0582d76
config: make the sanity check stronger
woodruffw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
NB: With
ensure_exists
this is now partially a no-op test, at least in local/CI testing (since these dirs don't exist). However for Cabotage/deployment it'll end up testing something useful, per cabotage/cabotage-app#85There 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.
just for clarity, we do not execute tests in the cabotage environment. should this be a runtime check?
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.
Ah, didn't realize that -- yeah, in that case it probably makes more sense as a runtime check. I'll look at putting it somewhere early in
python -m warehouse
's initialization 🙂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've put this in
warehouse/config.py
, as part of the early initialization that happens there. But I'm not 100% sure if that's the best place 🙂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.
In smoke testing this, it looks like this doesn't correctly detect that things are working.
I rolled out a new release of test.pypi.org using this wrapper and you can see results in cabotage/cabotage-app#85 (comment)
platformdirs.user_data_dir(ensure_exists=True)
succeeds, butplatformdirs.user_data_dir('app', ensure_exists=True)
fails, indicating that this wouldn't actually ensure success for xdg/platformdirs use later on.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.
appears this is due to the existing /tmp/{share, cache} files created by the Dockerfile on main :)
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.
validated this locally!
I still think it may be wise to change these to
platformdirs.user_cache_dir(secrets.token_urlsafe(), ensure_exists=True)
and similar to ensure not only that the dirs exist but that they're writable.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.
Whoops 🤦 -- I think merging this first should fix that then, since we won't be doing
mkdir
in the Dockerfile anymore.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.
Done!