Download and use the goma client from cipd #41488

zanderso · 2023-04-25T19:25:37Z

In flutter/flutter#125361 we discovered that a new version of clang can require updating goma in order for goma to work properly. This PR adds a dependency on CIPD goma to the DEPS file so that we can update it for use in local builds when needed. Since CI does its own management of the goma client and the compiler proxy, and since goma can only be used by Googlers, the DEPS file download is guarded behind the use_cipd_goma gclient var. To use it one would update the engine .gclient file to be something like:

~/flutter/engine/src $ cat ../.gclient
solutions = [
  {
    "managed": False,
    "name": "src/flutter",
    "url": "[email protected]:zanderso/engine.git",
    "custom_deps": {},
    "custom_vars": {
      "use_cipd_goma": True,
    },
    "deps_file": "DEPS",
    "safesync_url": "",
  },
]

I'll update the wiki with these instructions after this PR lands.

flutter-dashboard · 2023-04-25T22:27:00Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #41488 at sha 7fa6538

godofredoc · 2023-04-25T23:32:59Z

tools/gn

@@ -237,6 +266,9 @@ def setup_goma(args):
        os.getenv('FLUTTER_GOMA_CREATE_XCODE_SYMLINKS', '0') == '1'):
      goma_gn_args['create_xcode_symlinks'] = True

+  if goma_gn_args['use_goma'] and args.goma_ensure_start:
+    goma_ensure_start(goma_gn_args['goma_dir'])


Should this call be gated by an env variable to execute only on local builds? starting goma this way for CI builds will remove all the visibility from the goma recipe module and can potentially cause issues difficult to triage.

If we're un-forking the goma recipe module, then this should be gated to be only for local builds, but if we can't un-fork the recipe module, then driving goma from this script is one way we could get rid of the fork, and so avoid blocking clang rolls going forward.

I'll send the CL to prepare the goma module to unfork it when there is an arm64 cipd package

godofredoc · 2023-04-25T23:36:30Z

tools/gn

@@ -1070,6 +1102,19 @@ def parse_args(args):
      help='Do not run GN. Instead configure the Impeller cmake example build.',
  )

+  parser.add_argument(


Are we giving too much power to this script? by the name it seems it was originally designed as a gn wrapper but now it is doing gn, cmake, and goma.

By design, this is the build setup step that the engine_v2 recipe calls, so if we want to run something before we do a ninja build, this is where it has to go, right?

godofredoc

LGTM with a question about starting goma for non googlers

godofredoc · 2023-04-26T17:01:38Z

DEPS

@@ -965,6 +1010,36 @@ hooks = [
      'src/flutter/tools/activate_emsdk.py',
    ]
  },
+  {


Will this work for external users with no access to goma?

No, but this runhook will only run if the use_cipd_goma is explicitly set to true.

zanderso · 2023-04-26T17:02:30Z

Added stronger gating on the gclient var, and shifted goma_ctl.py ensure_start to a gclient runhook (also gated by the gclient var) to avoid running on every gn invocation.

dnfield · 2023-04-26T17:21:42Z

DEPS

+  # stops working on a clang roll, this may need to be updated using the value
+  # from the 'integration' tag of
+  # https://chrome-infra-packages.appspot.com/p/fuchsia/third_party/goma/client
+  'goma_version': ' git_revision:41b3bcb64014144a844153fd5588c36411fffb56',


not integration?

Are we updating the clang roller to update this?

Yeah that's a good idea. We can ask the Skia autoroller folks if that's possible.

Not integration because we should probably avoid unpinned dependencies.

dnfield · 2023-04-26T17:23:43Z

Making the compiler proxy start as a gclient hook rather than the GN script is an improvement

…41488)

…125583) flutter/engine@cf97541...fddd5ad 2023-04-26 [email protected] [Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend. (flutter/engine#41374) 2023-04-26 [email protected] Manual clang roll to 5344d8e10bb7d8672d4bfae8adb010465470d51b (flutter/engine#41520) 2023-04-26 [email protected] Download and use the goma client from cipd (flutter/engine#41488) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…41488)

…125593) flutter/engine@cf97541...d4ca524 2023-04-27 [email protected] Roll Skia from 20a1c61c5597 to d315ab065af3 (5 revisions) (flutter/engine#41535) 2023-04-26 [email protected] Revert "[Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend." (flutter/engine#41533) 2023-04-26 [email protected] [codesign] Add pinned xcode version as property to mac android aot engine (flutter/engine#41518) 2023-04-26 [email protected] [Impeller] Coerce opaque ColorSourceContents to Source (flutter/engine#41525) 2023-04-26 [email protected] Roll Skia from 3fea88565a83 to 20a1c61c5597 (3 revisions) (flutter/engine#41530) 2023-04-26 [email protected] [Impeller] partial repaint for Impeller/iOS. (flutter/engine#40959) 2023-04-26 [email protected] Updated todo with github issue link (flutter/engine#41517) 2023-04-26 [email protected] Roll Clang from 20d06c833d83 to 5344d8e10bb7 (flutter/engine#41524) 2023-04-26 [email protected] [Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend. (flutter/engine#41374) 2023-04-26 [email protected] Manual clang roll to 5344d8e10bb7d8672d4bfae8adb010465470d51b (flutter/engine#41520) 2023-04-26 [email protected] Download and use the goma client from cipd (flutter/engine#41488) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

zanderso force-pushed the cipd-goma branch 2 times, most recently from f382206 to 7fa6538 Compare April 25, 2023 21:46

flutter-dashboard bot added the will affect goldens label Apr 25, 2023

godofredoc reviewed Apr 25, 2023

View reviewed changes

zanderso added 2 commits April 26, 2023 09:00

Manual Clang roll

e1e07da

Download and use goma client from cipd

0f5dcd2

zanderso force-pushed the cipd-goma branch from 7fa6538 to 0f5dcd2 Compare April 26, 2023 16:51

zanderso requested a review from dnfield April 26, 2023 17:01

godofredoc approved these changes Apr 26, 2023

View reviewed changes

dnfield reviewed Apr 26, 2023

View reviewed changes

zanderso merged commit aff8cbe into flutter:main Apr 26, 2023

zanderso deleted the cipd-goma branch April 26, 2023 18:30

engine-flutter-autoroll mentioned this pull request Apr 26, 2023

Roll Flutter Engine from cf975417ce40 to fddd5add8b62 (3 revisions) flutter/flutter#125583

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 26, 2023

aff8cbe27 Download and use the goma client from cipd (flutter/engine#…

f242e1f

…41488)

engine-flutter-autoroll mentioned this pull request Apr 26, 2023

Roll Flutter Engine from cf975417ce40 to 464abc748021 (10 revisions) flutter/flutter#125592

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 26, 2023

aff8cbe27 Download and use the goma client from cipd (flutter/engine#…

d60b4ac

…41488)

engine-flutter-autoroll mentioned this pull request Apr 27, 2023

Roll Flutter Engine from cf975417ce40 to d4ca5240da52 (11 revisions) flutter/flutter#125593

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 27, 2023

aff8cbe27 Download and use the goma client from cipd (flutter/engine#…

a8d59e3

…41488)

zanderso mentioned this pull request Aug 11, 2023

Add goma warnings to the gn script flutter/flutter#117238

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Download and use the goma client from cipd #41488

Download and use the goma client from cipd #41488

Uh oh!

zanderso commented Apr 25, 2023 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Apr 25, 2023

Uh oh!

godofredoc Apr 25, 2023

Uh oh!

zanderso Apr 26, 2023 •

edited

Loading

Uh oh!

godofredoc Apr 26, 2023

Uh oh!

godofredoc Apr 25, 2023

Uh oh!

zanderso Apr 26, 2023

Uh oh!

godofredoc left a comment

Uh oh!

godofredoc Apr 26, 2023

Uh oh!

zanderso Apr 26, 2023

Uh oh!

zanderso commented Apr 26, 2023

Uh oh!

dnfield Apr 26, 2023

Uh oh!

zanderso Apr 26, 2023

Uh oh!

zanderso Apr 26, 2023

Uh oh!

dnfield commented Apr 26, 2023

Uh oh!

Uh oh!

Download and use the goma client from cipd #41488

Download and use the goma client from cipd #41488

Uh oh!

Conversation

zanderso commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Apr 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanderso Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godofredoc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanderso commented Apr 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Apr 26, 2023

Uh oh!

Uh oh!

zanderso commented Apr 25, 2023 •

edited

Loading

zanderso Apr 26, 2023 •

edited

Loading