engine icon indicating copy to clipboard operation
engine copied to clipboard

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

Open jmagman opened this issue 1 year ago • 3 comments

"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:

  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. 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 (https://github.com/flutter/engine/pull/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 https://github.com/flutter/engine/pull/56014#pullrequestreview-2385392205).

Part of https://github.com/flutter/flutter/issues/155041 Fixes https://github.com/flutter/flutter/issues/157275

Pre-launch Checklist

  • [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
  • [x] I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • [x] I listed at least one issue that this PR fixes in the description above.
  • [x] 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.
  • [x] I updated/added relevant documentation (doc comments with ///).
  • [x] I signed the CLA.
  • [x] All existing and new tests are passing.

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

jmagman avatar Oct 22 '24 05:10 jmagman

"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?

jmagman avatar Oct 22 '24 18:10 jmagman

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.

zanderso avatar Oct 22 '24 18:10 zanderso

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

christopherfujino avatar Oct 22 '24 19:10 christopherfujino