Skip to content

add .pub-cache back to .gitignore #128894

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.

Sign up for GitHub

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

Jump to bottom
Merged
auto-submit merged 3 commits into from
Jun 15, 2023
Merged

add .pub-cache back to .gitignore #128894

auto-submit merged 3 commits into from
Jun 15, 2023

Conversation

@christopherfujino
Copy link
Contributor

@christopherfujino christopherfujino commented Jun 14, 2023 •
edited
Loading

Fixes #128635

In 1fd84f8, we renamed the .pub-cache entry in the .gitignore file to .pub-preload-cache, and ensured the packaging script was now populating that directory. However, for users who already had this directory from downloading an earlier archive but used flutter upgrade to get the latest code, they now had a .gitignore file that was no longer ignoring their .pub-cache directory.

Now, when these users try to flutter upgrade again, the tool tries to verify if their checkout is clean, and will exit early if not directing the user to either stash or commit these changes, or do flutter upgrade --force which will try to update their branch anyway (and which would succeed since there would be no git conflict). These change adds back the .pub-cache entry to .gitignore, which won't retroactively fix broken releases, but will at least ensure if they flutter upgrade --force once to get this fix, they won't need to again.

@christopherfujino christopherfujino requested a review from Hixie June 14, 2023 18:51
@Hixie
Copy link
Contributor

Hixie commented Jun 14, 2023

test-exempt: configuration change

but bonus points for actually testing it. obviously the simple test is just literally checking the file contains what you expect, but the more elaborate test would be checking that the scenario you describe works as expected.

@christopherfujino
Copy link
Contributor Author

christopherfujino commented Jun 14, 2023

test-exempt: configuration change

but bonus points for actually testing it. obviously the simple test is just literally checking the file contains what you expect, but the more elaborate test would be checking that the scenario you describe works as expected.

I considered writing a test, but I'm not sure there would be much value. I'm not too worried about .pub-cache in particular regressing, ideally I would have a test that verifies we never remove anything from .gitignore, only add things (with a special allow-list for special cases where handle deleting the file from checkouts before/after upgrading).

However we don't currently have infra for seeing the diff associated with the current PR (we can't check the current commit because there may be multiple commits in a PR, and the HEAD commit may not have the offending change).

@christopherfujino
Copy link
Contributor Author

christopherfujino commented Jun 14, 2023

the more elaborate test would be checking that the scenario you describe works as expected.

This would be difficult to do. Ideally you would download a prebuilt archive and then flutter upgrade to the current commit you're testing, but then that feature branch commit won't exist upstream. And I think doing something like git remote add ... is a bad idea, and would break, as it's quite complicated all the places we fetch commits from across our CI processes.

Also we'd have the problem of which older archive to grab. The perfect test would grab all published release archives ever and ensure they can successfully upgrade to the commit under test.

@andrewkolos
Copy link
Contributor

andrewkolos commented Jun 14, 2023 •
edited
Loading

Would it be worth adding a comment to the .gitignore cautioning against removing entries?

(edited for typos)

@goderbauer goderbauer added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Jun 14, 2023
andrewkolos
andrewkolos reviewed Jun 14, 2023
View reviewed changes
Copy link
Contributor

@andrewkolos andrewkolos left a comment

Choose a reason for hiding this comment

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

See previous comment (I forgot to mark it as a review)

@christopherfujino
Copy link
Contributor Author

christopherfujino commented Jun 14, 2023

See previous comment (I forgot to mark it as a review)

Done

@github-actions github-actions bot removed the c: contributor-productivity Team-specific productivity, code health, technical debt. label Jun 14, 2023
andrewkolos
andrewkolos approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@andrewkolos andrewkolos left a comment

Choose a reason for hiding this comment

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

LGTM with uber nit

.gitignore Outdated
@@ -1,3 +1,6 @@
# Do not remove or rename entries in this file, only add new ones
# See https://github.com/flutter/flutter/issues/128635 for more context
Copy link
Contributor

@andrewkolos andrewkolos Jun 15, 2023

Choose a reason for hiding this comment

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

Suggested change
# See https://github.com/flutter/flutter/issues/128635 for more context
# See https://github.com/flutter/flutter/issues/128635 for more context.

@christopherfujino christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 15, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 15, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Jun 15, 2023

auto label is removed for flutter/flutter, pr: 128894, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@sigurdm
Copy link
Contributor

sigurdm commented Jun 15, 2023

In 1fd84f8, we renamed the .pub-cache entry in the .gitignore file to .pub-preload-cache

Just a small correction for later context. This was not a rename:

  • The FLUTTER_ROOT/.pub_cache was an actual pub cache that flutter pub would use
  • The .pub-preload-cache contains package archives, and on first run they are extracted to the user-level pub-cache (eg ~/.pub_cache on mac).

@andrewkolos andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 15, 2023
@auto-submit auto-submit bot merged commit d3e771c into flutter:master Jun 15, 2023
@christopherfujino
Copy link
Contributor Author

christopherfujino commented Jun 15, 2023

In 1fd84f8, we renamed the .pub-cache entry in the .gitignore file to .pub-preload-cache

Just a small correction for later context. This was not a rename:

  • The FLUTTER_ROOT/.pub_cache was an actual pub cache that flutter pub would use
  • The .pub-preload-cache contains package archives, and on first run they are extracted to the user-level pub-cache (eg ~/.pub_cache on mac).

Understood. I meant the effective change to the .gitignore, but I should have been more clear.

@christopherfujino christopherfujino deleted the add-pub-cache-to-gitignore branch June 15, 2023 19:49
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 16, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 16, 2023
@engine-flutter-autoroll
d3e771c add .pub-cache back to .gitignore (flutter/flutter#128894)
1870585
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 16, 2023
@engine-flutter-autoroll
Roll Flutter from b0188cd to fc8856e (14 revisions) (#4231)
08ab0af 
flutter/flutter@b0188cd...fc8856e

2023-06-16 [email protected] [web] Don't crash on `const HtmlElementView()` (flutter/flutter#128965)
2023-06-16 [email protected] Roll Packages from 0507297 to f9314a3 (3 revisions) (flutter/flutter#128878)
2023-06-16 [email protected] Update getProperties to handle Diagnosticable as input. (flutter/flutter#128897)
2023-06-15 [email protected] Roll Flutter Engine from 48e0b4e66422 to fb5fed432e59 (1 revision) (flutter/flutter#128967)
2023-06-15 [email protected] Fix dart pub cache clean command on pub.dart (flutter/flutter#128171)
2023-06-15 [email protected] [flutter_tools] Migrate more integration tests to process result matcher (flutter/flutter#128737)
2023-06-15 [email protected] [flutter_tools] refactor license collector (flutter/flutter#128748)
2023-06-15 [email protected] Set Semantics.button to true for date widget (flutter/flutter#128824)
2023-06-15 [email protected] Update golden tests (flutter/flutter#128914)
2023-06-15 [email protected] Roll Flutter Engine from 9934c0de738c to 48e0b4e66422 (26 revisions) (flutter/flutter#128959)
2023-06-15 [email protected] flutter update-packages --cherry-pick-package (flutter/flutter#128917)
2023-06-15 [email protected] add .pub-cache back to .gitignore (flutter/flutter#128894)
2023-06-15 [email protected] Roll Flutter Engine from 2d8d5ecfe4a8 to 9934c0de738c (2 revisions) (flutter/flutter#128849)
2023-06-15 [email protected] flutter update-packages --force-upgrade (flutter/flutter#128908)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023
@engine-flutter-autoroll
d3e771c add .pub-cache back to .gitignore (flutter/flutter#128894)
7ee4009
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
d3e771c add .pub-cache back to .gitignore (flutter/flutter#128894)
c027e7f
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
d3e771c add .pub-cache back to .gitignore (flutter/flutter#128894)
3014134
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
d3e771c add .pub-cache back to .gitignore (flutter/flutter#128894)
1576356
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewkolos andrewkolos andrewkolos approved these changes

@Hixie Hixie Awaiting requested review from Hixie

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Flutter upgrade ko for .pub-cache folder

5 participants

@christopherfujino @Hixie @andrewkolos @sigurdm @goderbauer