Skip to content
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.

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

Clean up gn script goma logic a bit #40817

Merged
merged 1 commit into from Mar 31, 2023
Merged

Conversation

zanderso
Copy link
Member

No description provided.

@zanderso zanderso merged commit 571baf1 into flutter:main Mar 31, 2023
38 checks passed
@zanderso zanderso deleted the gn-goma-error branch March 31, 2023 15:23
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 31, 2023
@whesse
Copy link
Contributor

whesse commented Apr 19, 2023

There was a big effort in Dart to stop using goma from the HOME dir and always use it from the depot_tools cipd checkout, following a request from the depot_tools team, a few years ago.

None of our CI uses goma from the HOME dir, or installs it there. I don't think removing support for the depot_tools location is a good idea, especially if support for the HOME location as a default is left in.

This is currently breaking the tip-of-tree builds of Flutter on our benchmarking system, but I think we will fix it by explicitly disabling goma use.

@athomas

goma_gn_args['use_goma'] = True
goma_gn_args['goma_dir'] = goma_home_dir
elif args.goma:
raise Exception(
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously, there was a fallback to non-goma builds if GOMA could not be found, rather than a hard failure.
This change to a hard failure may break some CI systems.

@whesse
Copy link
Contributor

whesse commented Apr 19, 2023

The specification of the --depot-tools argument to this script says that it is an alternative path that is searched for gomacc. So that documentation should be changed as well.

@zanderso
Copy link
Member Author

If --goma is specified, and isn't found, we no longer fall back because engine developers (esp. new team members) were confused by the soft failure and subsequent frustratingly slow local builds.

@whesse Why not just set the environment variable on your CI to the goma location? I believe that is how Flutter CI works.

@whesse
Copy link
Contributor

whesse commented Apr 19, 2023

We have added --no-goma where needed. There is no problem with this change sticking, if it is intended to remove the fallback to no Goma. I'm surprised it removes the fallback to depot-tools Goma, because that is what Dart uses, and has removed support for Goma in $HOME, but that may be best if Flutter uses only Fuchsia's Goma with a different server hardcoded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants