-
Notifications
You must be signed in to change notification settings - Fork 6k
use host_cpu in BUILD.gn to determine where to find gen_snapshot #34870
use host_cpu in BUILD.gn to determine where to find gen_snapshot #34870
Conversation
f4fc35c to
e6d65c6
Compare
zanderso
left a comment
There was a problem hiding this 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) |
There was a problem hiding this comment.
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'.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Fixes: flutter/flutter#108238