Add standalone 'Mac clangd' builder to replace 'Linux mac_clangd' orchestrator #56014

jmagman · 2024-10-22T05:07:58Z

"Orchestrator" builders are top-level builds that can perform some caching, kick off "drone" sub-builds, and then do things with those build artifacts like run tests, or "generators" that do work on those artifacts. See more details in https://flutter.dev/go/engine-build-definition-language. Linux mac_clangd is currently set up as an orchestrator builder, but it only kicks off one gn and one test that can be converted to a standalone build. Linux mac_clangd is confusingly named because the orchestrator is a Linux machine, but the drone is a Mac.

Copy Linux mac_clangd into ci/builders/standalone and reference it in a new Mac clangd build in bringup. Note since we want to rename the builder it can't replace Linux mac_clangd in the same commit. I will follow up and remove Linux mac_clangd once Mac clangd bringup is removed. Unfortunately that means we lose commit history.

Differences from ci/builders/mac_unopt_debug_no_rbe.json to the standalone version:

Remove the drone_dimensions device_type and os and $flutter/osx_sdk property sdk_version since they are already set via the "Mac" builder name. Remove mac_model since there's no reason to specify hardware except for benchmarks afaik. Macmini8,1 used to need to be specified to run on an Intel Mac before the cpu property was supported.
Remove timeout (default back to 30 minutes) since the timeout was added for the cold cache orchestrator timeouts (Increase timeout for Linux mac_clangd #55988).

I repurposed non-bringup Mac mac_unopt to test the standalone config (and then reverted it) (result). Here's the passing clangd test: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8733402970006982913/+/u/test:_clangd/stdout

I also renamed linux_unopt_debug_no_rbe.json to linux_clangd.json to match and added cas_archive (see #56014 (review)).

Part of flutter/flutter#155041
Fixes flutter/flutter#157275

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…estrator

jmagman · 2024-10-22T05:41:32Z

ci/builders/standalone/mac_unopt_debug_no_rbe.json

@@ -0,0 +1,39 @@
+{


@zanderso should this be named mac_unopt_debug_no_rbe.json or something like mac_clangd.json since the comment says not to use it for anything else?

That make sense to me. The name of the Linux one should probably also be changed.

zanderso

"cas_archive": false, should be retained in the build definition. It should probably also be added to the other standalone builds since it defaults to true: https://flutter.googlesource.com/recipes/+/refs/heads/main/recipes/engine_v2/builder.py#244.

zanderso · 2024-10-22T14:27:42Z

ci/builders/standalone/mac_unopt_debug_no_rbe.json

@@ -0,0 +1,39 @@
+{


That make sense to me. The name of the Linux one should probably also be changed.

jmagman · 2024-10-22T18:08:28Z

"cas_archive": false, should be retained in the build definition. It should probably also be added to the other standalone builds since it defaults to true:

Ah I thought there was no need to do this since the archiving was specifically so the subbuilds archived for the orchestrators to pull down. @christopherfujino Do you know what the standalone builds use CAS caching for?

zanderso · 2024-10-22T18:12:50Z

Ah I thought there was no need to do this since the archiving was specifically so the subbuilds archived for the orchestrators to pull down. @christopherfujino Do you know what the standalone builds use CAS caching for?

It can be useful as a debugging tool because if something goes wrong you can download all the artifacts and inspect them locally. But that's something that we can enable case-by-case instead of having it on all the time.

christopherfujino · 2024-10-22T19:07:55Z

Ah I thought there was no need to do this since the archiving was specifically so the subbuilds archived for the orchestrators to pull down. @christopherfujino Do you know what the standalone builds use CAS caching for?

It can be useful as a debugging tool because if something goes wrong you can download all the artifacts and inspect them locally. But that's something that we can enable case-by-case instead of having it on all the time.

TIL

zanderso

Thanks!

christopherfujino

LGTM (so I can blame myself should this break a release :) )

…langd' orchestrator (flutter/engine#56014)

…157486) flutter/engine@5d7caf7...0b56cb8 2024-10-24 [email protected] Bare-bones iOS FKA implementation (flutter/engine#55964) 2024-10-23 [email protected] [Impeller] libImpeller: Allow appending to the transformation stack. (flutter/engine#56072) 2024-10-23 [email protected] Add standalone 'Mac clangd' builder to replace 'Linux mac_clangd' orchestrator (flutter/engine#56014) 2024-10-23 [email protected] Roll Dart SDK from 75c42f30af7a to dd06a1e3002c (2 revisions) (flutter/engine#56070) 2024-10-23 [email protected] Roll Skia from 42f9070e6625 to 53c9663c3b83 (1 revision) (flutter/engine#56069) 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] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Follow up to #56014 `Mac clangd` is [passing as a standalone build](https://ci.chromium.org/ui/p/flutter/builders/staging/Mac%20clangd/60/overview) (<10 minutes). Move it out of bringup and remove `Linux mac_clangd`. flutter/flutter#157275 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…lutter#157486) flutter/engine@5d7caf7...0b56cb8 2024-10-24 [email protected] Bare-bones iOS FKA implementation (flutter/engine#55964) 2024-10-23 [email protected] [Impeller] libImpeller: Allow appending to the transformation stack. (flutter/engine#56072) 2024-10-23 [email protected] Add standalone 'Mac clangd' builder to replace 'Linux mac_clangd' orchestrator (flutter/engine#56014) 2024-10-23 [email protected] Roll Dart SDK from 75c42f30af7a to dd06a1e3002c (2 revisions) (flutter/engine#56070) 2024-10-23 [email protected] Roll Skia from 42f9070e6625 to 53c9663c3b83 (1 revision) (flutter/engine#56069) 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] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Build and x64 dimension was introduced in #50901, and I blindly copied it into the .ci.yaml in #56014. However I don't think clangd needs to run on the scarce (and flakier) Intel Macs (37 x64 bots in prod vs 55 arm bots). Suspect this was copied from clang_tidy #26910 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…hestrator (flutter/engine#56014) "Orchestrator" builders are top-level builds that can perform some caching, kick off "drone" sub-builds, and then do things with those build artifacts like run tests, or "generators" that do work on those artifacts. See more details in https://flutter.dev/go/engine-build-definition-language. `Linux mac_clangd` is currently set up as an orchestrator builder, but it only kicks off one gn and one test that can be converted to a standalone build. `Linux mac_clangd` is confusingly named because the orchestrator is a Linux machine, but the drone is a Mac. Copy `Linux mac_clangd` into [`ci/builders/standalone`](https://github.com/flutter/engine/tree/c17390e606bf1ef5d64caf34c2e6d2fb5800fc1a/ci/builders/standalone) and reference it in a new `Mac clangd` build in `bringup`. Note since we want to rename the builder it can't replace `Linux mac_clangd` in the same commit. I will follow up and remove `Linux mac_clangd` once `Mac clangd` `bringup` is removed. Unfortunately that means we lose commit history. Differences from [ci/builders/mac_unopt_debug_no_rbe.json](https://github.com/flutter/engine/blob/9a7492dc886374eff8f44f47b1a8eb73bba6bc13/ci/builders/mac_unopt_debug_no_rbe.json) to the standalone version: 1. Remove the drone_dimensions `device_type` and `os` and `$flutter/osx_sdk` property `sdk_version` since they are [already set via the "Mac" builder name](https://github.com/flutter/engine/blob/c17390e606bf1ef5d64caf34c2e6d2fb5800fc1a/.ci.yaml#L26-L37). Remove `mac_model` since there's no reason to specify hardware except for benchmarks afaik. `Macmini8,1` used to need to be specified to run on an Intel Mac before the `cpu` property was supported. 2. Remove `timeout` (default back to 30 minutes) since the timeout was added for the cold cache orchestrator timeouts (flutter/engine#55988). I repurposed non-bringup `Mac mac_unopt` to test the standalone config (and then reverted it) ([result](https://ci.chromium.org/ui/p/flutter/builders/try/Mac%20mac_unopt/10479/infra)). Here's the passing clangd test: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8733402970006982913/+/u/test:_clangd/stdout I also renamed linux_unopt_debug_no_rbe.json to linux_clangd.json to match and added cas_archive (see flutter/engine#56014 (review)). Part of flutter#155041 Fixes flutter#157275 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

) Follow up to flutter/engine#56014 `Mac clangd` is [passing as a standalone build](https://ci.chromium.org/ui/p/flutter/builders/staging/Mac%20clangd/60/overview) (<10 minutes). Move it out of bringup and remove `Linux mac_clangd`. flutter#157275 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Build and x64 dimension was introduced in flutter/engine#50901, and I blindly copied it into the .ci.yaml in flutter/engine#56014. However I don't think clangd needs to run on the scarce (and flakier) Intel Macs (37 x64 bots in prod vs 55 arm bots). Suspect this was copied from clang_tidy flutter#26910 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

jmagman force-pushed the mac-clangd branch from b2822f1 to 7ceee62 Compare October 22, 2024 05:11

Add standalone 'Mac clang' builder to replace 'Linux mac_clangd' orch…

d371fdb

…estrator

jmagman force-pushed the mac-clangd branch from 7ceee62 to d371fdb Compare October 22, 2024 05:16

jmagman mentioned this pull request Oct 22, 2024

Consider shifting Linux mac_clangd to the "standalone" recipe flutter/flutter#157275

Closed

mac_unopt test

5687dec

jmagman force-pushed the mac-clangd branch from a2f5d8f to 5687dec Compare October 22, 2024 05:23

jmagman added the test: all See https://github.com/flutter/engine/blob/main/docs/ci/Engine-pre-submits-and-post-submits.md label Oct 22, 2024

jmagman added 2 commits October 21, 2024 22:26

empty

99a9f78

mac_unopt

bf396ad

jmagman removed the test: all See https://github.com/flutter/engine/blob/main/docs/ci/Engine-pre-submits-and-post-submits.md label Oct 22, 2024

mac_unopt

14c2804

jmagman marked this pull request as ready for review October 22, 2024 05:40

jmagman commented Oct 22, 2024

View reviewed changes

jmagman self-assigned this Oct 22, 2024

jmagman requested a review from zanderso October 22, 2024 05:42

jmagman changed the title ~~Add standalone 'Mac clang' builder to replace 'Linux mac_clangd' orchestrator~~ Add standalone 'Mac clangd' builder to replace 'Linux mac_clangd' orchestrator Oct 22, 2024

zanderso reviewed Oct 22, 2024

View reviewed changes

jmagman added 3 commits October 22, 2024 20:02

Review edits

4175dff

use_cas

60abd00

cas_archive

c7374ab

zanderso approved these changes Oct 23, 2024

View reviewed changes

christopherfujino approved these changes Oct 23, 2024

View reviewed changes

jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 23, 2024

auto-submit bot merged commit b4ef615 into flutter:main Oct 23, 2024
35 checks passed

jmagman deleted the mac-clangd branch October 23, 2024 23:03

engine-flutter-autoroll mentioned this pull request Oct 24, 2024

Roll Flutter Engine from 5d7caf771022 to 0b56cb8de79e (5 revisions) flutter/flutter#157486

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 24, 2024

b4ef615a3 Add standalone 'Mac clangd' builder to replace 'Linux mac_c…

62cbe0e

…langd' orchestrator (flutter/engine#56014)

This was referenced Oct 24, 2024

Remove 'Linux mac_clangd' in favor of 'Mac clangd' #56082

Merged

Proposal: Remove engine top-level orchestrator builders that only kick off and collect sub-builds (no generators), and hoist sub-build to "standalone" flutter/flutter#155041

Open

jmagman mentioned this pull request Nov 13, 2024

Move mac clangd build from x64 to arm #56567

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add standalone 'Mac clangd' builder to replace 'Linux mac_clangd' orchestrator #56014

Add standalone 'Mac clangd' builder to replace 'Linux mac_clangd' orchestrator #56014

jmagman commented Oct 22, 2024 •

edited

Loading

jmagman Oct 22, 2024 •

edited

Loading

zanderso Oct 22, 2024 •

edited

Loading

zanderso left a comment

zanderso Oct 22, 2024 •

edited

Loading

jmagman commented Oct 22, 2024

zanderso commented Oct 22, 2024

christopherfujino commented Oct 22, 2024

zanderso left a comment

christopherfujino left a comment

Add standalone 'Mac clangd' builder to replace 'Linux mac_clangd' orchestrator #56014

Add standalone 'Mac clangd' builder to replace 'Linux mac_clangd' orchestrator #56014

Conversation

jmagman commented Oct 22, 2024 • edited Loading

Pre-launch Checklist

jmagman Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

zanderso Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

zanderso left a comment

Choose a reason for hiding this comment

zanderso Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

jmagman commented Oct 22, 2024

zanderso commented Oct 22, 2024

christopherfujino commented Oct 22, 2024

zanderso left a comment

Choose a reason for hiding this comment

christopherfujino left a comment

Choose a reason for hiding this comment

jmagman commented Oct 22, 2024 •

edited

Loading

jmagman Oct 22, 2024 •

edited

Loading

zanderso Oct 22, 2024 •

edited

Loading

zanderso Oct 22, 2024 •

edited

Loading