fix(tool): Use merge-base for content hash in detached HEAD #175554
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with incorrect content-aware hash calculation in detached HEAD states during local development, which caused problems like failed Dart SDK downloads. The change modifies bin/internal/content_aware_hash.sh to distinguish between a local detached HEAD and a CI environment by checking the LUCI_CI environment variable. My review found a high-severity issue: the corresponding PowerShell script, content_aware_hash.ps1, was not updated with the same logic. This breaks the explicit requirement for script consistency and means the fix will not work on Windows. This needs to be addressed to ensure cross-platform compatibility.
b51584b to
4645173
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adjusts the content-aware hashing scripts (.sh and .ps1) to correctly handle detached HEAD states, which is a common scenario for developers using tools like jj. The change correctly differentiates between a local detached HEAD and a CI environment by checking for the LUCI_CI environment variable. This ensures the merge-base is used for hashing content locally to avoid unnecessary rebuilds, while maintaining the existing behavior on CI. The logic implemented in both scripts is correct and effectively addresses the issue. I have one suggestion for the shell script to enhance the readability of the new conditional logic.
4645173 to
116c637
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where the content-aware hash generation script incorrectly handled detached HEAD states in local development environments, particularly when using tools like jj. The fix correctly differentiates between a local detached HEAD and a CI environment by checking for the LUCI_CI environment variable. This change is applied to both the shell and PowerShell scripts, and you've added a new test case to validate the behavior in a local detached HEAD scenario, which is great. My review includes one suggestion to improve the readability of the conditional logic in the PowerShell script.
| ($currentBranch -ne "stable") -and | ||
| ($currentBranch -ne "beta") -and | ||
| ($currentBranch -ne "HEAD") -and | ||
| (-not (($currentBranch -eq "HEAD") -and (-not [string]::IsNullOrEmpty($env:LUCI_CI)))) -and |
There was a problem hiding this comment.
While the logic here is correct, the nested negations make it a bit difficult to parse. You can simplify this expression by applying De Morgan's laws. The condition (-not (A -and (-not B))) is equivalent to ((not A) or B), which is more direct and easier to read.
(($currentBranch -ne "HEAD") -or [string]::IsNullOrEmpty($env:LUCI_CI)) -and
There was a problem hiding this comment.
I disagree. I think the condition, as is, is clearer, since if you translate it directly to plain English it is "not (current branch is "HEAD" and LUCI_CI is set)" which accurately describes what we are checking for.
There was a problem hiding this comment.
I will second making sure this matches the bash scripting.
The `content_aware_hash.sh` script determines which version of the engine to use. For local development, it uses the merge-base with the remote tracking branch to avoid unnecessary rebuilds. However, when using `jj`, the underlying git repository is in a detached HEAD state. The script was incorrectly interpreting this as a CI environment and was not calculating the hash based on the merge-base, leading to incorrect engine versions and failed Dart SDK downloads. This change modifies the script to differentiate between a local detached HEAD state (like with `jj`) and a CI environment by checking for the `LUCI_CI` environment variable. This ensures the correct engine hash is generated for both local `jj` users and CI builds.
116c637 to
1135871
Compare
|
First, thank you for investing in making this better! Have we agreed to support |
+1, thank you!
I'm not aware of any decision one way or another. If it's something we can make work without breaking existing |
| "$CURRENT_BRANCH" != "gh-readonly-queue/master/pr-"* && \ | ||
| "$CURRENT_BRANCH" != "flutter-"*"-candidate."* && \ | ||
| "$CURRENT_BRANCH" != "HEAD" && \ | ||
| ! ( "$CURRENT_BRANCH" == "HEAD" && -n "$LUCI_CI" ) && \ |
There was a problem hiding this comment.
ok, I checked out HEAD~ to get into detached head (while on master) just to see if this would still compute. tl/dr it does. Why this is important: people still need to do bisect.
What was the problem with LUCI_CI?
There was a problem hiding this comment.
The problem arises when you are in a detached HEAD state and also have branched off of main. Being in detached HEAD but still on a commit in main will work because there will be a Dart SDK available to download with that content hash. But if you are in detached HEAD and also branched off main, then there is no Dart SDK uploaded for that content hash and so downloading the Dart SDK will fail. I assume it works on LUCI because the builders put themselves in a detached HEAD state by checking out the contents of a PR, but in this case a Dart SDK for that content hash has already been uploaded in an earlier builder step.
There was a problem hiding this comment.
Pretty please keep an eye on this when it lands.
Roll Flutter from 8f94cb0d8f01 to 9ff2767f3cb6 (56 revisions) flutter/flutter@8f94cb0...9ff2767 2025-09-20 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 0_jKqLGnkILvQ5C8a... to CcCe3HpQtBYhTZscb... (flutter/flutter#175698) 2025-09-20 engine-flutter-autoroll@skia.org Roll Dart SDK from e6e9248aee4f to 9e943fe076c8 (1 revision) (flutter/flutter#175697) 2025-09-20 32538273+ValentinVignal@users.noreply.github.com Add `menuController` to `DropdownMenu` (flutter/flutter#175039) 2025-09-20 engine-flutter-autoroll@skia.org Roll Skia from 1dae085e2f31 to a38a531dec1d (3 revisions) (flutter/flutter#175694) 2025-09-20 bruno.leroux@gmail.com [a11y] TimePicker clock is unnecessarily announced (flutter/flutter#175570) 2025-09-20 engine-flutter-autoroll@skia.org Roll Dart SDK from 78e68d1a7dbf to e6e9248aee4f (4 revisions) (flutter/flutter#175690) 2025-09-19 engine-flutter-autoroll@skia.org Roll Skia from b56003bf2c20 to 1dae085e2f31 (1 revision) (flutter/flutter#175674) 2025-09-19 matanlurey@users.noreply.github.com Update `CODEOWNERS` (for dev-tooling) (flutter/flutter#175201) 2025-09-19 sokolovskyi.konstantin@gmail.com [a11y-app] Add label to TextFormField in AutoCompleteUseCase. (flutter/flutter#175576) 2025-09-19 sokolovskyi.konstantin@gmail.com Fix RadioGroup single selection check. (flutter/flutter#175654) 2025-09-19 engine-flutter-autoroll@skia.org Roll Packages from f2a65fd to 3d5c419 (2 revisions) (flutter/flutter#175668) 2025-09-19 engine-flutter-autoroll@skia.org Roll Skia from c74d2bdbd93c to b56003bf2c20 (2 revisions) (flutter/flutter#175665) 2025-09-19 32538273+ValentinVignal@users.noreply.github.com Add `CupertinoLinearActivityIndicator` (flutter/flutter#170108) 2025-09-19 bkonyi@google.com [ Widget Preview ] Don't update filtered preview set when selecting non-source files (flutter/flutter#175596) 2025-09-19 engine-flutter-autoroll@skia.org Roll Dart SDK from 2c79803c97db to 78e68d1a7dbf (3 revisions) (flutter/flutter#175646) 2025-09-19 mdebbar@google.com Delete unused web_unicode library (flutter/flutter#174896) 2025-09-19 engine-flutter-autoroll@skia.org Roll Skia from 684f3a831216 to c74d2bdbd93c (2 revisions) (flutter/flutter#175644) 2025-09-19 engine-flutter-autoroll@skia.org Roll Skia from 462bdece17bf to 684f3a831216 (3 revisions) (flutter/flutter#175641) 2025-09-19 engine-flutter-autoroll@skia.org Roll Skia from a2c38aa9df80 to 462bdece17bf (11 revisions) (flutter/flutter#175629) 2025-09-18 1961493+harryterkelsen@users.noreply.github.com fix(tool): Use merge-base for content hash in detached HEAD (flutter/flutter#175554) 2025-09-18 1961493+harryterkelsen@users.noreply.github.com [web] Unskip Cupertino datepicker golden tests in Skwasm (flutter/flutter#174666) 2025-09-18 ryjohn@google.com Update rules to include extension rules (flutter/flutter#175618) 2025-09-18 Breakthrough@users.noreply.github.com [engine] Cleanup Fuchsia FDIO library dependencies (flutter/flutter#174847) 2025-09-18 ahmedsameha1@gmail.com Make sure that a CloseButton doesn't crash in 0x0 environment (flutter/flutter#172902) 2025-09-18 bkonyi@google.com [ Tool ] Serve DevTools from DDS, remove ResidentDevToolsHandler (flutter/flutter#174580) 2025-09-18 Breakthrough@users.noreply.github.com [engine][fuchsia] Update to Fuchsia API level 28 and roll latest GN SDK (flutter/flutter#175425) 2025-09-18 jessiewong401@gmail.com Added a 36 device for Firebase Lab Testing (flutter/flutter#175613) 2025-09-18 engine-flutter-autoroll@skia.org Roll Dart SDK from 09a101793af4 to 2c79803c97db (2 revisions) (flutter/flutter#175608) 2025-09-18 louisehsu@google.com Engine Support for Dynamic View Resizing (flutter/flutter#173610) 2025-09-18 engine-flutter-autoroll@skia.org Roll Packages from fdee698 to f2a65fd (3 revisions) (flutter/flutter#175594) 2025-09-18 15619084+vashworth@users.noreply.github.com Connect the FlutterEngine to the FlutterSceneDelegate (flutter/flutter#174910) 2025-09-18 engine-flutter-autoroll@skia.org Roll Dart SDK from de5dd0f1530f to 09a101793af4 (2 revisions) (flutter/flutter#175583) 2025-09-18 engine-flutter-autoroll@skia.org Roll Skia from 7b9fe91446ee to a2c38aa9df80 (1 revision) (flutter/flutter#175579) 2025-09-18 engine-flutter-autoroll@skia.org Roll Skia from ab1b10547461 to 7b9fe91446ee (4 revisions) (flutter/flutter#175569) 2025-09-18 bruno.leroux@gmail.com [a11y-app] Fix form field label and error message (flutter/flutter#174831) 2025-09-18 bruno.leroux@gmail.com Fix InputDecoration does not apply errorStyle to error (flutter/flutter#174787) 2025-09-18 32538273+ValentinVignal@users.noreply.github.com Migrate to `WidgetPropertyResolver` (flutter/flutter#175397) 2025-09-18 32538273+ValentinVignal@users.noreply.github.com Migrate to WidgetState (flutter/flutter#175396) 2025-09-18 engine-flutter-autoroll@skia.org Roll Skia from 79ec8dfcd9d4 to ab1b10547461 (17 revisions) (flutter/flutter#175561) 2025-09-18 30870216+gaaclarke@users.noreply.github.com Removes NOTICES from licenses input (flutter/flutter#174967) 2025-09-18 engine-flutter-autoroll@skia.org Roll Dart SDK from 116f7fe72839 to de5dd0f1530f (2 revisions) (flutter/flutter#175557) 2025-09-17 1961493+harryterkelsen@users.noreply.github.com [reland][web] Refactor renderers to use the same frontend code #174588 (flutter/flutter#175392) 2025-09-17 erickzanardoo@gmail.com feat: Enable WidgetStateColor to be used in ChipThemeData.deleteIconColor (flutter/flutter#171646) 2025-09-17 15619084+vashworth@users.noreply.github.com Filter out unexpected process logs on iOS with better regex matching. (flutter/flutter#175452) 2025-09-17 victorsanniay@gmail.com CupertinoContextMenu child respects available screen width (flutter/flutter#175300) 2025-09-17 38427679+xVemu@users.noreply.github.com Correct documentation in PredictiveBackFullscreenPageTransitionsBuilder (flutter/flutter#174362) ...
…175554) The `content_aware_hash.sh` script determines which version of the engine to use. For local development, it uses the merge-base with the remote tracking branch to avoid unnecessary rebuilds. However, when using `jj`, the underlying git repository is in a detached HEAD state. The script was incorrectly interpreting this as a CI environment and was not calculating the hash based on the merge-base, leading to incorrect engine versions and failed Dart SDK downloads. This change modifies the script to differentiate between a local detached HEAD state (like with `jj`) and a CI environment by checking for the `LUCI_CI` environment variable. This ensures the correct engine hash is generated for both local `jj` users and CI builds. Here is an example of failing to download the Dart SDK before: ``` Downloading Darwin arm64 Dart SDK from Flutter engine f6ea244d7b75547c2c1a4613299b24dcebe3ce5c... % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 258 100 258 0 0 858 0 --:--:-- --:--:-- --:--:-- 857 [/Users/het/Projects/flutter/bin/cache/dart-sdk-darwin-arm64.zip] End-of-central-directory signature not found. Either this file is not a zipfile, or it constitutes one disk of a multi-part archive. In the latter case the central directory and zipfile comment will be found on the last disk(s) of this archive. unzip: cannot find zipfile directory in one of /Users/het/Projects/flutter/bin/cache/dart-sdk-darwin-arm64.zip or /Users/het/Projects/flutter/bin/cache/dart-sdk-darwin-arm64.zip.zip, and cannot find /Users/het/Projects/flutter/bin/cache/dart-sdk-darwin-arm64.zip.ZIP, period. It appears that the downloaded file is corrupt; please try again. If this problem persists, please report the problem at: https://github.com/flutter/flutter/issues/new?template=01_activation.yml ``` ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
|
I should have caught this, but 'LUCI_CI' is not an environment variable used anywhere. I believe it should have been |
The
content_aware_hash.shscript determines which version of the engine to use. For local development, it uses the merge-base with the remote tracking branch to avoid unnecessary rebuilds.However, when using
jj, the underlying git repository is in a detached HEAD state. The script was incorrectly interpreting this as a CI environment and was not calculating the hash based on the merge-base, leading to incorrect engine versions and failed Dart SDK downloads.This change modifies the script to differentiate between a local detached HEAD state (like with
jj) and a CI environment by checking for theLUCI_CIenvironment variable. This ensures the correct engine hash is generated for both localjjusers and CI builds.Here is an example of failing to download the Dart SDK before:
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.