Skip to content

Conversation

@jtmcdole
Copy link
Member

@jtmcdole jtmcdole commented Aug 2, 2025

The content hash doesn't exist for local engine changes, except for on
CI. If we detect we're on a branch with committed or uncommitted changes
to engine files; use "master".

towards #171790

re-land attempt for #173114 (try 2)
re-land attempt for #172792 (original)

The first commit in this PR is the previously LGTM'd changes for the above; with tests.

The second commit is the critical change to make this work in post submits (fixes #173143). It turns out that while LUCI reports the GitHub private branches; our recipes directly checkout the git sha. This matters because the content scripts couldn't determine the branch name and the rev-parse was just HEAD. This lead the scripts down the merge-base logic, which returns the previous commit.

A test was added specifically for this.

Alternatively to this change, we could have checked for LUCI_CONTEXT being present in the environment. This is checked by Flutter tools in some cases, but not by any other scripts in bin/internal. The downside to checking HEAD: if you have a local branch with engine changes and you move back a revision - dart/flutter invocations will generate the hash for your local changes and fail.

The content hash doesn't exist for local engine changes, except for on CI. If we detect we're on a branch with committed or uncommitted changes to engine files; use "master".

towards flutter#171790

re-land attempt three: luci context
@github-actions github-actions bot added tool Affects the "flutter" command-line tool. See also t: labels. team-tool Owned by Flutter Tool team d: docs/ flutter/flutter/docs, for contributors labels Aug 2, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request addresses an issue with content hash generation for local engine branches by correctly identifying the base for hashing. The changes introduce logic to use the merge-base with master or main for development branches, while correctly handling release branches, CI environments with detached HEADs, and shallow clones. The PowerShell and Bash scripts are updated consistently, and the changes are well-supported by a comprehensive suite of new tests in the Dart test file. Additionally, the documentation has been updated to reflect this new hashing logic. My review includes a few suggestions to improve the readability of the shell scripts and a minor code cleanup.

The main cause for failing in post submit.
@jtmcdole jtmcdole force-pushed the localHashBypassPartTrois branch from 4e5f43a to 2fa6211 Compare August 2, 2025 21:28
@jtmcdole jtmcdole added this pull request to the merge queue Aug 4, 2025
@jtmcdole
Copy link
Member Author

jtmcdole commented Aug 4, 2025

Queueing so I can get an early warning with an auto roller

Merged via the queue into flutter:master with commit 7da78cb Aug 4, 2025
148 checks passed
@jtmcdole jtmcdole deleted the localHashBypassPartTrois branch August 4, 2025 00:36
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 4, 2025
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Aug 4, 2025
Roll Flutter from 871849e4b6bf to beda687d63f2 (34 revisions)

flutter/flutter@871849e...beda687

2025-08-04 [email protected] [Impeller] Improvements to the Vulkan pipeline cache data writer (flutter/flutter#173014)
2025-08-04 [email protected] [Impeller] Terminate the fence waiter but do not reset it during ContextVK shutdown (flutter/flutter#173085)
2025-08-04 [email protected] Add the 'windowing' feature flag and use to wrap an implementation for regular windows that always throws (flutter/flutter#172478)
2025-08-04 [email protected] licenses_cpp: reland switch 4 (flutter/flutter#173139)
2025-08-04 [email protected] Roll Skia from 763bba9c33fd to dce9550a1356 (1 revision) (flutter/flutter#173219)
2025-08-04 [email protected] Improve robustness of comment detection when using flutter analyze --suggestions (flutter/flutter#172977)
2025-08-04 [email protected] Make sure that a LicensePage doesn't crash in 0x0 environment (flutter/flutter#172610)
2025-08-04 [email protected] Roll Packages from f0645d8 to 1a72287 (4 revisions) (flutter/flutter#173215)
2025-08-04 [email protected] Roll Skia from edf0f8a5bba6 to 763bba9c33fd (1 revision) (flutter/flutter#173213)
2025-08-04 [email protected] [web] Add Intl.Locale to parse browser languages. (flutter/flutter#172964)
2025-08-04 [email protected] Roll Skia from 439f80973f4a to edf0f8a5bba6 (2 revisions) (flutter/flutter#173204)
2025-08-04 [email protected] Roll Skia from 9d30cc96a3b2 to 439f80973f4a (1 revision) (flutter/flutter#173201)
2025-08-04 [email protected] Roll Skia from 39c70f883c0e to 9d30cc96a3b2 (1 revision) (flutter/flutter#173197)
2025-08-04 [email protected] Roll Skia from 09667386bcba to 39c70f883c0e (1 revision) (flutter/flutter#173194)
2025-08-04 [email protected] fix: get content hash for master on local engine branches (third attempt)  (flutter/flutter#173169)
2025-08-03 [email protected] Roll Fuchsia Linux SDK from wZuA8Dqty4scQkZuV... to ufssK8EgJ_9RpLFgu... (flutter/flutter#173190)
2025-08-03 [email protected] Roll Skia from e056e47e118b to 09667386bcba (2 revisions) (flutter/flutter#173185)
2025-08-03 [email protected] fix: tag fuchsia package after uploading (flutter/flutter#173140)
2025-08-02 [email protected] Roll Skia from 7ef888e30a28 to e056e47e118b (1 revision) (flutter/flutter#173170)
2025-08-02 [email protected] Roll Fuchsia Linux SDK from yFLr81lLXmSX7n11D... to wZuA8Dqty4scQkZuV... (flutter/flutter#173166)
2025-08-02 [email protected] Roll Skia from af6d6eb383a6 to 7ef888e30a28 (1 revision) (flutter/flutter#173157)
2025-08-02 [email protected] Add `innerRadius` to `RadioThemeData` (flutter/flutter#173120)
2025-08-02 [email protected] Roll Skia from 81dd6aa516b0 to af6d6eb383a6 (2 revisions) (flutter/flutter#173144)
2025-08-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts "fix: get content hash for master on local engine branches (#173114)" (flutter/flutter#173145)
2025-08-01 [email protected] Roll Dart SDK from 6e1bb159c860 to 3f1307d72d6f (2 revisions) (flutter/flutter#173136)
2025-08-01 [email protected] Roll Skia from 9e2c59634961 to 81dd6aa516b0 (1 revision) (flutter/flutter#173135)
2025-08-01 [email protected] Reformat text.dart's code snippets (flutter/flutter#172976)
2025-08-01 [email protected] experiment with docs properties (flutter/flutter#173124)
2025-08-01 [email protected] fix: get content hash for master on local engine branches (flutter/flutter#173114)
2025-08-01 [email protected] Make sure that a RawAutocomplete doesn't crash in 0x0 environment (flutter/flutter#172812)
2025-08-01 [email protected] Add skia_ports_fontmgr_android_parser_sources (flutter/flutter#172979)
2025-08-01 [email protected] Roll Skia from 49e39cd3cf18 to 9e2c59634961 (10 revisions) (flutter/flutter#173125)
2025-08-01 [email protected] fix: 🐛 Add equality and hashCode implementations to ResizeImage (flutter/flutter#172643)
2025-08-01 [email protected] Roll Fuchsia Linux SDK from xo_bqOoFuBKFmgKxn... to yFLr81lLXmSX7n11D... (flutter/flutter#173117)

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] 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://issues.skia.org/issues/new?component=1389291&template=1850622

...
danilozhang pushed a commit to danilozhang/flutter that referenced this pull request Aug 6, 2025
…mpt) (flutter#173169)

The content hash doesn't exist for local engine changes, except for on
CI. If we detect we're on a branch with committed or uncommitted changes
to engine files; use "master".

towards flutter#171790

re-land attempt for flutter#173114 (try 2)
re-land attempt for flutter#172792 (original)

The first commit in this PR is the previously LGTM'd changes for the
above; with tests.

The second commit is the critical change to make this work in post
submits (fixes flutter#173143). It turns out that while LUCI reports the GitHub
private branches; our recipes directly checkout the git sha. This
matters because the content scripts couldn't determine the branch name
and the rev-parse was just HEAD. This lead the scripts down the
merge-base logic, which returns the previous commit.

A test was added specifically for this.

Alternatively to this change, we could have checked for LUCI_CONTEXT
being present in the environment. This is checked by Flutter tools in
some cases, but not by any other scripts in `bin/internal`. The downside
to checking HEAD: if you have a local branch with engine changes and you
move back a revision - `dart`/`flutter` invocations will generate the
hash for your local changes and fail.
SydneyBao pushed a commit to SydneyBao/flutter that referenced this pull request Aug 6, 2025
…mpt) (flutter#173169)

The content hash doesn't exist for local engine changes, except for on
CI. If we detect we're on a branch with committed or uncommitted changes
to engine files; use "master".

towards flutter#171790

re-land attempt for flutter#173114 (try 2)
re-land attempt for flutter#172792 (original)

The first commit in this PR is the previously LGTM'd changes for the
above; with tests.

The second commit is the critical change to make this work in post
submits (fixes flutter#173143). It turns out that while LUCI reports the GitHub
private branches; our recipes directly checkout the git sha. This
matters because the content scripts couldn't determine the branch name
and the rev-parse was just HEAD. This lead the scripts down the
merge-base logic, which returns the previous commit.

A test was added specifically for this.

Alternatively to this change, we could have checked for LUCI_CONTEXT
being present in the environment. This is checked by Flutter tools in
some cases, but not by any other scripts in `bin/internal`. The downside
to checking HEAD: if you have a local branch with engine changes and you
move back a revision - `dart`/`flutter` invocations will generate the
hash for your local changes and fail.
ksokolovskyi pushed a commit to ksokolovskyi/flutter that referenced this pull request Aug 19, 2025
…mpt) (flutter#173169)

The content hash doesn't exist for local engine changes, except for on
CI. If we detect we're on a branch with committed or uncommitted changes
to engine files; use "master".

towards flutter#171790

re-land attempt for flutter#173114 (try 2)
re-land attempt for flutter#172792 (original)

The first commit in this PR is the previously LGTM'd changes for the
above; with tests.

The second commit is the critical change to make this work in post
submits (fixes flutter#173143). It turns out that while LUCI reports the GitHub
private branches; our recipes directly checkout the git sha. This
matters because the content scripts couldn't determine the branch name
and the rev-parse was just HEAD. This lead the scripts down the
merge-base logic, which returns the previous commit.

A test was added specifically for this.

Alternatively to this change, we could have checked for LUCI_CONTEXT
being present in the environment. This is checked by Flutter tools in
some cases, but not by any other scripts in `bin/internal`. The downside
to checking HEAD: if you have a local branch with engine changes and you
move back a revision - `dart`/`flutter` invocations will generate the
hash for your local changes and fail.
mboetger pushed a commit to mboetger/flutter that referenced this pull request Sep 18, 2025
…mpt) (flutter#173169)

The content hash doesn't exist for local engine changes, except for on
CI. If we detect we're on a branch with committed or uncommitted changes
to engine files; use "master".

towards flutter#171790

re-land attempt for flutter#173114 (try 2)
re-land attempt for flutter#172792 (original)

The first commit in this PR is the previously LGTM'd changes for the
above; with tests.

The second commit is the critical change to make this work in post
submits (fixes flutter#173143). It turns out that while LUCI reports the GitHub
private branches; our recipes directly checkout the git sha. This
matters because the content scripts couldn't determine the branch name
and the rev-parse was just HEAD. This lead the scripts down the
merge-base logic, which returns the previous commit.

A test was added specifically for this.

Alternatively to this change, we could have checked for LUCI_CONTEXT
being present in the environment. This is checked by Flutter tools in
some cases, but not by any other scripts in `bin/internal`. The downside
to checking HEAD: if you have a local branch with engine changes and you
move back a revision - `dart`/`flutter` invocations will generate the
hash for your local changes and fail.
korca0220 pushed a commit to korca0220/flutter that referenced this pull request Sep 22, 2025
…mpt) (flutter#173169)

The content hash doesn't exist for local engine changes, except for on
CI. If we detect we're on a branch with committed or uncommitted changes
to engine files; use "master".

towards flutter#171790

re-land attempt for flutter#173114 (try 2)
re-land attempt for flutter#172792 (original)

The first commit in this PR is the previously LGTM'd changes for the
above; with tests.

The second commit is the critical change to make this work in post
submits (fixes flutter#173143). It turns out that while LUCI reports the GitHub
private branches; our recipes directly checkout the git sha. This
matters because the content scripts couldn't determine the branch name
and the rev-parse was just HEAD. This lead the scripts down the
merge-base logic, which returns the previous commit.

A test was added specifically for this.

Alternatively to this change, we could have checked for LUCI_CONTEXT
being present in the environment. This is checked by Flutter tools in
some cases, but not by any other scripts in `bin/internal`. The downside
to checking HEAD: if you have a local branch with engine changes and you
move back a revision - `dart`/`flutter` invocations will generate the
hash for your local changes and fail.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 12, 2025
lucaantonelli pushed a commit to lucaantonelli/flutter that referenced this pull request Nov 21, 2025
…mpt) (flutter#173169)

The content hash doesn't exist for local engine changes, except for on
CI. If we detect we're on a branch with committed or uncommitted changes
to engine files; use "master".

towards flutter#171790

re-land attempt for flutter#173114 (try 2)
re-land attempt for flutter#172792 (original)

The first commit in this PR is the previously LGTM'd changes for the
above; with tests.

The second commit is the critical change to make this work in post
submits (fixes flutter#173143). It turns out that while LUCI reports the GitHub
private branches; our recipes directly checkout the git sha. This
matters because the content scripts couldn't determine the branch name
and the rev-parse was just HEAD. This lead the scripts down the
merge-base logic, which returns the previous commit.

A test was added specifically for this.

Alternatively to this change, we could have checked for LUCI_CONTEXT
being present in the environment. This is checked by Flutter tools in
some cases, but not by any other scripts in `bin/internal`. The downside
to checking HEAD: if you have a local branch with engine changes and you
move back a revision - `dart`/`flutter` invocations will generate the
hash for your local changes and fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

d: docs/ flutter/flutter/docs, for contributors team-tool Owned by Flutter Tool team tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

android tests are all red

4 participants