Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@flar
Copy link
Contributor

@flar flar commented Jul 24, 2022

@flar flar requested a review from zanderso July 26, 2022 02:52
@flar flar changed the title use platform.machine() to determine where to find gen_snapshot use host_cpu in BUILD.gn to determine where to find gen_snapshot Jul 26, 2022
@flar flar force-pushed the gate-gen_snapshot_dir-on-platform branch from f4fc35c to e6d65c6 Compare July 26, 2022 04:36
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.

lgtm after fix noted below. I'll follow-up with changes to the GN to compute the value for the flag instead of hardcoding it.

)

parser.add_argument('--dst', type=str, required=True)
parser.add_argument('--clang-dir', type=str, required=True)
Copy link
Member

Choose a reason for hiding this comment

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

The bots call this script, so to make this a soft transition, the new flag would need to be option with a default value of 'clang_x64'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yay! That explains the mystery errors that had no logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this begs the question - if/when we start having Apple Silicon machines running our CI, won't this just shift the problem from needing a default to needing the create_macos_gen_snapshots.py script be able to figure out the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or teaching the CI machines to figure this out and provide the answer when they call the script? Maybe that's the best strategy?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we'll have to modify the recipe to get it to work on arm64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it worth updating the recipes now?

Copy link
Member

Choose a reason for hiding this comment

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

After this lands, we should update to explicitly pass the new flag, but I think we should wait to add logic to programatically figure out the arch until we're ready to bring those machines online.

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 26, 2022
@auto-submit auto-submit bot merged commit 356ab75 into flutter:main Jul 26, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 27, 2022
jwoelke added a commit to LibertyGlobal/flutter-tvos-engine that referenced this pull request May 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Engine builds for Android on Mac always look for gen_snapshot in the default clang_x64 directory

2 participants