Skip to content

Use LOCALAPPDATA for system cache on windows #2297

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 4 commits into from
Jan 9, 2020

Conversation

Jesse-Bakker
Copy link
Contributor

@Jesse-Bakker Jesse-Bakker commented Jan 7, 2020

Currently, the default location for pub's system cache is in %APPDATA%, which means it is in the roaming profile that gets synced across devices. This is less than ideal because it can get pretty large.

This PR moves the default location of the cache to %LOCALAPPDATA%, but will use %APPDATA% if the cache already exists there to avoid breaking existing installs linking to that location

Fixes #1273

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@Jesse-Bakker
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@Jesse-Bakker Jesse-Bakker force-pushed the windows-local-appdata branch from 1166f02 to f872018 Compare January 7, 2020 11:41
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jan 7, 2020
@kevmoo kevmoo requested a review from jonasfj January 7, 2020 16:18
Copy link
Member

@jonasfj jonasfj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice fix... I only have two nits, if you don't do those we'll probably do it and land..

Note to self: ensure we have a good commit message so we can explain this in dart-sdk CHANGELOG when we roll pub in...

var appData = Platform.environment['APPDATA'];
return p.join(appData, 'Pub', 'Cache');
var appDataCacheDir = p.join(appData, 'Pub', 'Cache');
if (Directory(appDataCacheDir).existsSync()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, but for consistency with the rest of the code base can we consider using dirExists(dir).

@@ -35,8 +35,14 @@ class SystemCache {
if (Platform.environment.containsKey('PUB_CACHE')) {
return Platform.environment['PUB_CACHE'];
} else if (Platform.isWindows) {
// If a cache dir already exists in %APPDATA%, use it, else default to %LOCALAPPDATA%
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, Can we briefly explain that we prefer %APPDATA% if it already exists because older pub versions created the pub-cache here.
And maybe state that using %LOCALAPPDATA% is better because it's not synchronized.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jan 8, 2020
@Jesse-Bakker Jesse-Bakker force-pushed the windows-local-appdata branch from f26a056 to f650df5 Compare January 8, 2020 16:05
Copy link
Member

@jonasfj jonasfj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+

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

Successfully merging this pull request may close these issues.

pub uses %APPDATA% and not %LOCALAPPDATA%
3 participants