Skip to content

Always use user-level pub cache #121802

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 8 commits into from
Mar 13, 2023
Merged

Always use user-level pub cache #121802

merged 8 commits into from
Mar 13, 2023

Conversation

sigurdm
Copy link
Contributor

@sigurdm sigurdm commented Mar 2, 2023

Current flutter has a .pub_cache inside the flutter_root and merges that with ~/.pub_cache if that exists,

With this PR we will always use pubs own resolution for finding the pub cache.
To add packages to the flutter.zip download they are packaged as tar.gz and added to the pub-cache on first run by using the new pub cache preload functionality.

Fixes #37481

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Mar 2, 2023
@sigurdm sigurdm requested a review from jonasfj March 2, 2023 15:43
@sigurdm
Copy link
Contributor Author

sigurdm commented Mar 6, 2023

@godofredoc you might want to look at the changes to the release packaging script dev/bots/prepare_package.dart.

@sigurdm sigurdm requested a review from godofredoc March 6, 2023 11:37
@sigurdm sigurdm changed the title Always use local cache Always use user-level pub cache Mar 6, 2023
@sigurdm sigurdm marked this pull request as ready for review March 6, 2023 11:54
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.

LGTM

/// These archives will be installed in the user-level cache on first
/// following flutter command that accesses the cache.
///
/// This assumes that all packages currently in the cache are installed from
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a safe assumption? What about users overriding PUB_HOSTED_URL? https://docs.flutter.dev/community/china#configuring-flutter-to-use-a-mirror-site

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this assumption is only as a precondition to the function, inside the prepare_package script.

Edited the comment to clarify.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I missed what library we were in :)

Future<void> _downloadPubPackageArchives() async {
final Pool pool = Pool(10); // Number of simultaneous downloads.
final http.Client client = http.Client();
final Directory preloadCache = Directory(path.join(flutterRoot.path, '.pub-preload-cache'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add this to the top-level repo .gitignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! good point.
Done

@christopherfujino
Copy link
Contributor

merged in upstream to fix linux build tests

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

@Jasguerrero can you give final approval?

Copy link
Contributor

@Jasguerrero Jasguerrero left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

DBC

Did we manually inspect the results of prepare_package.dart to ensure that it has the same size and rough contents?

There are also many style issues in this PR. In particular, the style guide asks us to avoid breaking a line on an assignment operator. See: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#prefer-a-maximum-line-length-of-80-characters

final String name = package.key;
final Map<String, dynamic> versions = package.value as Map<String, dynamic>;
for (final String version in versions.keys) {
downloads.add(fetchPackageArchive(name, version));
Copy link
Member

Choose a reason for hiding this comment

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

If the pub cache on CI has multiple versions of the same package, will they all be included in the flutter.zip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They should, as we iterate all versions for each package, and we store them as $name-$version.tar.gz.

Copy link
Member

Choose a reason for hiding this comment

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

Is that the same as the old behavior, or will the release .zip now contain many versions of the same package that aren't needed. Remember that the flutter/flutter pubspec.yaml files pin all packages to a single version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should have exactly the same set of package-versions as before.

The difference is that they are distributed as tar.gz files instead of inside a prepared cache.

We do the same steps to "warm up" a cache (by running pub get for some set of packages) and then, instead of folding that cache inside the zip, we iterate it and download the archive for each package.

Copy link
Contributor

Choose a reason for hiding this comment

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

DBC

Did we manually inspect the results of prepare_package.dart to ensure that it has the same size and rough contents?

I'm working on this currently, will update.

Copy link
Contributor

Choose a reason for hiding this comment

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

I ran prepare_package.dart before and after this change on the same beta commit:

dart prepare_package.dart --branch=beta --no-publish --revision=1a0a03a41d6613c10bab23508dee456f184b181b --temp_dir=$HOME/git/tmp/prepare_package

And verified that the set of packages are the same, as well as the versions. The only difference is that after this change the individual packages are stored as tarballs.

@sigurdm
Copy link
Contributor Author

sigurdm commented Mar 13, 2023

There are also many style issues in this PR. In particular, the style guide asks us to avoid breaking a line on an assignment operator. See: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#prefer-a-maximum-line-length-of-80-characters

If this sticks I can do a follow-up to clean up.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 13, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 13, 2023
@sigurdm sigurdm mentioned this pull request Mar 14, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2023
@gilice gilice mentioned this pull request Apr 2, 2023
12 tasks
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
auto-submit bot pushed a commit that referenced this pull request May 16, 2023
Follow-up to #121802 resolving some style issues.
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
Follow-up to flutter#121802 resolving some style issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flutter's local .pub-cache doesn't interact well with separate Dart install
5 participants