-
-
Notifications
You must be signed in to change notification settings - Fork 388
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
Use cache dirs rather than data dirs, and allow path customisation. #791
Conversation
…a store to the cache location.
src/briefcase/commands/base.py
Outdated
# isn't defined, use a platform-specific default data path. | ||
if data_path is None: | ||
try: | ||
data_path = os.environ["BRIEFCASE_HOME"] |
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 think it might be good to ensure that whatever is in BRIEFCASE_HOME
can be created as a directory or already exists as a directory via mkdir(parents=True, exists_ok=True)
....otherwise, the bomb out later could be other than intuitive. (may even be good to see if touch()
is successful within the directory but that may be overkill.)
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 was partially intentional; if the user specifies something incorrect, I don't want to start arbitrarily creating directories. However, verifying that the directory actually exists is a good idea.
appauthor="BeeWare", | ||
).user_cache_path | ||
|
||
if " " in os.fsdecode(data_path): |
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.
If we really want to be absolute, checking all characters against string.whitespace
may be best.....but I think I say this mostly tongue in cheek.
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.
Yeah... but you just know someone is going to put a tab in their username for giggles...
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.
or a non-breaking space ;)
src/briefcase/commands/base.py
Outdated
if data_path is None: | ||
try: | ||
data_path = Path(os.environ["BRIEFCASE_HOME"]) | ||
if not data_path.exists(): |
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.
Unfortunately, Path("")
returns PosixPath('.')
....so this lets an empty BRIEFCASE_HOME
sneak by. I wouldn't consider this especially important if I hadn't tried export <ENV VAR>=""
too many times in the past instead of unset <ENV VAR>
.
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.
Good catch - I've added an explicit catch of this case.
Codecov Report
|
#777 modified the location of the Briefcase data store from
~/.briefcase
to a platform-specific data folder.However, it was then discovered that Android tooling cannot be stored in a location that has spaces in the path (#789). This is a problem because the default data location on macOS is
~/Library/Application Support
.To address this, this PR modifies the default storage location from the user data folder to the user cache folder. This is
~/Library/Caches
on macOS, so it is "Android safe". The cache folder was originally considered as a location, but rejected based on a comment about how the caches folder may be arbitrarily deleted. However, further investigation shows no evidence that this happens arbitrarily as a result of OS operation; and by way of social proof, pip also uses the cache location.Another benefit of the caches locations is that
~/Library/Application Support
is a location that is backed up by default by macOS, whereas~/Library/Caches
is not; so this gives a small benefit to those using macOS backup services.Another related change - macOS convention is to use a bundle ID as part of the cache path; this isn't explicitly supported by platformdirs, but it's easy to add.
However, this change by itself won't fix the problem if the user has a space in their username. While this is uncommon, it is possible; to avoid the problem, this PR also introduces a
BRIEFCASE_HOME
environment variable that lets the user specify an arbitrary path for their Briefcase data. While this is a direct workaround for the Android "space-in-path" problem, it has the added benefit that it allows users that have disk space concerns to use an alternate storage location for Briefcase content.Checks have been added to ensure that the user doesn't have a space in their data path.
PR Checklist: