[flutter_tools] join flutter specific with home cache #105343

Jasguerrero · 2022-06-03T23:02:27Z

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
All existing and new tests are passing.

jonasfj · 2022-06-07T11:17:58Z

packages/flutter_tools/lib/src/dart/pub.dart

Logic for finding the default PUB_CACHE is here:
https://github.com/dart-lang/pub/blob/73ea9a98b5bef9cd2b1ca4d1eba8b6d20e1ad3d0/lib/src/system_cache.dart#L40-L60

There is occasionally some discussion around moving to a more XDG compliant location. But that's not happening right now. Feel free to file a PR against pub with a //NOTE: .... saying that when changing this we should also patch flutter_tools.

jonasfj · 2022-06-07T11:20:23Z

Isn't there also an opportunity to cleanup:

flutter/bin/internal/shared.sh

Lines 152 to 154 in be0c1bd

    
           if [[ -d "$FLUTTER_ROOT/.pub-cache" ]]; then 
        
             export PUB_CACHE="${PUB_CACHE:-"$FLUTTER_ROOT/.pub-cache"}" 
        
           fi

(and similar thing in the .bat variant)

christopherfujino · 2022-06-07T00:02:23Z

packages/flutter_tools/lib/src/dart/pub.dart

commented out code

christopherfujino · 2022-06-07T00:04:57Z

packages/flutter_tools/lib/src/dart/pub.dart

You can't hard code '/' as this will be wrong on Windows. You can copy the rest of this file, where _fileSystem.path.join(...) is called.

Also, will detecting the home directory work this way on Windows?

christopherfujino · 2022-06-07T00:09:03Z

packages/flutter_tools/lib/src/dart/pub.dart

Suggested change

void joinCaches(FileSystem fileSystem, String targetPath, String extraPath) {

void joinCaches({

required FileSystem fileSystem,

required String destinationPath,

required String sourcePath,

) {

christopherfujino · 2022-06-07T16:53:53Z

packages/flutter_tools/lib/src/dart/pub.dart

readAsBytesSync -> writeAsBytesSync will pull the entire file into memory before writing it again; this will use a lot of resources and be slow; use https://api.dart.dev/stable/2.17.3/dart-io/File/renameSync.html (per that dartdoc, if the source and destination paths are on different filesystems, that may fail, so maybe copy + delete would be safer?)

christopherfujino · 2022-06-07T16:54:51Z

packages/flutter_tools/lib/src/dart/pub.dart

A FileSystemEntity doesn't have to be a file, I think instead you need to check if entity is File

christopherfujino · 2022-06-10T18:16:33Z

packages/flutter_tools/lib/src/dart/pub.dart

I think we can probably just recursively delete the entire local cache at the end.

I didn't want to face the issue of running out of memory. But if we don't think that will happen a lot I can delete this line

(doing many little deletes will be much slower than a single file system call)

Jasguerrero · 2022-06-10T18:18:01Z

packages/flutter_tools/lib/src/dart/pub.dart

Isn't there also an opportunity to cleanup:

flutter/bin/internal/shared.sh

Lines 152 to 154 in be0c1bd

if [[ -d "$FLUTTER_ROOT/.pub-cache" ]]; then

export PUB_CACHE="${PUB_CACHE:-"$FLUTTER_ROOT/.pub-cache"}"

fi

(and similar thing in the .bat variant)

@jonasfj
As I mention in this comment we are not actually deleting the FLUTTER_ROOT/.pub-cache always, we are just doing that when FLUTTER_ROOT/.pub-cache and HOME/.pub-cache are available in the users environment so I don't think deleting the PUB-CACHE variable will be correct

christopherfujino · 2022-06-10T18:18:17Z

packages/flutter_tools/lib/src/dart/pub.dart

style nit: de-dent once and add a trailing comma to line 62

You added the comma, but not the de-dent

christopherfujino · 2022-06-10T18:18:31Z

packages/flutter_tools/lib/src/dart/pub.dart

style nit: de-dent once and add a trailing comma to line 555

Here, and lines 564, 567, 571-573, please de-dent once and add trailing commas where there aren't one already

christopherfujino · 2022-06-10T18:19:54Z

packages/flutter_tools/lib/src/dart/pub.dart

nit: can you update the parameter names for joinCaches to be globalCachePath and localCachePath. Also change the local variables here to match.

christopherfujino · 2022-06-10T18:20:30Z

packages/flutter_tools/lib/src/dart/pub.dart

style nit: this else if should be on the previous line, after the close curly brace

Jasguerrero · 2022-06-10T18:22:21Z

packages/flutter_tools/lib/src/dart/pub.dart

@jonasfj I copy paste this comment from the dart-lang repo. Also can you double check on this logic for windows? Thanks.

christopherfujino · 2022-06-10T18:29:25Z

packages/flutter_tools/lib/src/dart/pub.dart

This is a surprising API to me. would it make sense to require the caller to pass a path already including .pub-cache?

Actually this needs to change after my last commit. Sorry will update it

christopherfujino · 2022-06-10T18:30:46Z

packages/flutter_tools/lib/src/dart/pub.dart

I think you can make this code simpler by skipping this check, and instead going directly down to pub.dartlang.org dir and checking that those exist.

packages/flutter_tools/lib/src/dart/pub.dart

christopherfujino · 2022-06-10T18:36:33Z

packages/flutter_tools/lib/src/dart/pub.dart

Same notes about dartdocs should start with a high level summary: https://dart.dev/guides/language/effective-dart/documentation#do-start-doc-comments-with-a-single-sentence-summary

christopherfujino · 2022-06-10T18:37:13Z

packages/flutter_tools/lib/src/dart/pub.dart

Is this indented wrong?

packages/flutter_tools/lib/src/dart/pub.dart

christopherfujino · 2022-06-13T20:39:31Z

packages/flutter_tools/lib/src/dart/pub.dart

I don't think '' is a valid path, right?

It is not. What should happens when _platform.environment['HOME'] (or 'LOCALAPPDATA'/'APPDATA') is null? Is it ok to fail? (_platform.environment['HOME']!)

no, it is valid to not have a home directory. Maybe globalDirectory should be nullable, and in this case it would be null. And then later you treat a null globalDirectory the same as if it didn't exist.

christopherfujino · 2022-06-13T20:41:55Z

packages/flutter_tools/lib/src/dart/pub.dart

Suggested change

/// There is 3 ways to get the pub cache location

/// There are 3 ways for the tool to find a pub cache location:

christopherfujino · 2022-06-13T20:43:14Z

packages/flutter_tools/lib/src/dart/pub.dart

Suggested change

/// 2) localPath (flutterRoot) but not globalPath ($HOME) .pubCache in which case we use the available (and vice versa).

/// 2) There is a local cache (in the Flutter SDK) but not a global one (in the user's home directory).

christopherfujino · 2022-06-13T20:43:44Z

packages/flutter_tools/lib/src/dart/pub.dart

Suggested change

/// 3) If both local and global are available joinCaches is called and local deleted

/// 3) If both local and global are available then merge the local into global and return the global.

christopherfujino · 2022-06-16T21:07:26Z

packages/flutter_tools/lib/src/dart/pub.dart

can't you just use entity itself?

christopherfujino · 2022-06-16T21:08:25Z

packages/flutter_tools/lib/src/dart/pub.dart

you can just use entity as a file in this block, because of type promotion

christopherfujino · 2022-06-16T21:11:15Z

packages/flutter_tools/lib/src/dart/pub.dart

I don't think this can ever not exist--feel free to explain if i'm reading this wrong.

christopherfujino · 2022-06-16T21:17:09Z

packages/flutter_tools/lib/src/dart/pub.dart

We need to check if globalDirectory exists, and if not, return the local cache path

christopherfujino · 2022-06-16T21:20:23Z

packages/flutter_tools/lib/src/dart/pub.dart

Can we make pubCache non-nullable, and always tell pub where we know the pub cache is?

The problem comes if the localPubCache and the globalPubCache are both empty we still need to depend on pub's logic to create the global and skip setting the PUB_CACHE environment variable

can we rename the function _getPubCacheIfAvailable() and have it return the global cache if we have one?

…r/flutter#105343)

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 3, 2022

Jasguerrero changed the title ~~joinCaches function~~ [flutter_tool] join flutter specific with home cache Jun 6, 2022

Jasguerrero changed the title ~~[flutter_tool] join flutter specific with home cache~~ [flutter_tools] join flutter specific with home cache Jun 6, 2022

Jasguerrero marked this pull request as ready for review June 6, 2022 20:38

Jasguerrero requested a review from christopherfujino June 6, 2022 20:38

jonasfj reviewed Jun 7, 2022

View reviewed changes

christopherfujino suggested changes Jun 7, 2022

View reviewed changes

christopherfujino reviewed Jun 10, 2022

View reviewed changes

Jasguerrero commented Jun 10, 2022

View reviewed changes

christopherfujino reviewed Jun 10, 2022

View reviewed changes

Jasguerrero commented Jun 10, 2022

View reviewed changes

christopherfujino reviewed Jun 10, 2022

View reviewed changes

packages/flutter_tools/lib/src/dart/pub.dart Outdated Show resolved Hide resolved

christopherfujino reviewed Jun 10, 2022

View reviewed changes

packages/flutter_tools/lib/src/dart/pub.dart Outdated

Copy link

Contributor

christopherfujino Jun 10, 2022

Choose a reason for hiding this comment

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

Is this indented wrong?

christopherfujino reviewed Jun 13, 2022

View reviewed changes

packages/flutter_tools/lib/src/dart/pub.dart Outdated Show resolved Hide resolved

christopherfujino reviewed Jun 13, 2022

View reviewed changes

christopherfujino reviewed Jun 16, 2022

View reviewed changes

packages/flutter_tools/lib/src/dart/pub.dart Outdated

Copy link

Contributor

christopherfujino Jun 16, 2022

Choose a reason for hiding this comment

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

can't you just use entity itself?

christopherfujino reviewed Jun 16, 2022

View reviewed changes

Jasguerrero added 17 commits July 29, 2022 12:02

style changes

9514315

update windows get home directory

2031504

remove extra space

56c584e

fix join caches fn

8f4716f

update fn name to retreive pub cache location

2054ae9

rename tests

f55edc3

Add test for joining pub caches

e7f28eb

add files to tests

32153ef

try catch on failed directory while joining caches

72f7d6b

make global cache deletion inside the joinCaches fn

7cac09b

update to catch the file exception outside the joincache fn

09158ea

add recursive to the deletion

360548c

create newDirectory in a different fn

470758a

update tests to only copy dependencies directory

9d6dc76

remove unused variable

e3e4207

format on exception clauses

eecf7fb

make fn to delete both caches local and global dependency

f8db52f

Jasguerrero reopened this Jul 29, 2022

Jasguerrero added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 29, 2022

auto-submit bot merged commit 6048e07 into flutter:master Jul 29, 2022

engine-flutter-autoroll mentioned this pull request Jul 30, 2022

Roll Flutter from a62ee852950a to 176f7d71306b (28 revisions) flutter/plugins#6161

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jul 30, 2022

6048e07 [flutter_tools] join flutter specific with home cache (flutte…

07c1910

…r/flutter#105343)

engine-flutter-autoroll mentioned this pull request Jul 30, 2022

Roll Flutter from a62ee852950a to 176f7d71306b (28 revisions) flutter/packages#2399

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 30, 2022

6048e07 [flutter_tools] join flutter specific with home cache (flutte…

fbe895e

…r/flutter#105343)

engine-flutter-autoroll mentioned this pull request Jul 30, 2022

Roll Flutter from a62ee852950a to ab47f01ff173 (29 revisions) flutter/plugins#6162

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jul 30, 2022

6048e07 [flutter_tools] join flutter specific with home cache (flutte…

efa05a8

…r/flutter#105343)

sigurdm mentioned this pull request Aug 8, 2022

[meta] Aligning 'flutter pub' and 'dart pub' behavior dart-lang/pub#2971

Open

9 tasks

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022

[flutter_tools] join flutter specific with home cache (flutter#105343)

95b669b

Jasguerrero deleted the flutter_pub_cache branch September 28, 2022 23:06

sigurdm mentioned this pull request Jan 4, 2024

flutter create emits confusing language about caches unprompted #140628

Closed

-void joinCaches(FileSystem fileSystem, String targetPath, String extraPath) {
+void joinCaches({
+  required FileSystem fileSystem,
+  required String destinationPath,
+  required String sourcePath,
+) {

	if [[ -d "$FLUTTER_ROOT/.pub-cache" ]]; then
	export PUB_CACHE="${PUB_CACHE:-"$FLUTTER_ROOT/.pub-cache"}"
	fi

	/// There is 3 ways to get the pub cache location
	/// There are 3 ways for the tool to find a pub cache location:

	/// 2) localPath (flutterRoot) but not globalPath ($HOME) .pubCache in which case we use the available (and vice versa).
	/// 2) There is a local cache (in the Flutter SDK) but not a global one (in the user's home directory).

	/// 3) If both local and global are available joinCaches is called and local deleted
	/// 3) If both local and global are available then merge the local into global and return the global.

[flutter_tools] join flutter specific with home cache #105343

[flutter_tools] join flutter specific with home cache #105343

Uh oh!

Conversation

Jasguerrero commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasfj commented Jun 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jasguerrero Jun 10, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino Jun 10, 2022 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jasguerrero commented Jun 3, 2022 •

edited

Loading

Jasguerrero Jun 10, 2022 •

edited

Loading

christopherfujino Jun 10, 2022 •

edited

Loading