Update engine build config docs. #41468

godofredoc · 2023-04-25T00:38:44Z

Removed sections relevan only to a design doc.
Remove deprecated properties.
Add docs about default properites.
Rephrased some sections.

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 Hixie said 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.

* Removed sections relevan only to a design doc. * Remove deprecated properties. * Add docs about default properites. * Rephrased some sections.

zanderso

Not sure I caught everywhere, but since the canonical home for the source is in GitHub, links to the code should be GitHub URLs rather than codesearch URLs.

Many small nits, but there are also several places where more details about options, their possible values, and their meanings would be helpful.

ci/builders/README.md

zanderso · 2023-05-03T20:32:24Z

ci/builders/README.md

-* **Parameters**, flags passed to the script. Both input and output paths must be relative to the checkout directory.
+* **Name** - the name of the step running the script.
+* **Parameters** - flags passed to the script. Both input and output paths must
+be relative to the checkout directory.
 * **Script**, the script path relative to the checkout repository.
 * **Language**, the script language executable to run the script. If empty it is assumed to be bash.


The bit about the default being bash was missing from the description of the language field above, I think.

Added the default value description to the tests section.

ci/builders/README.md

zanderso · 2023-05-03T20:39:41Z

ci/builders/README.md


+```yaml
+"archives": [


I'm a bit confused about how/why this differs from the archives list in sub-builds. The archives components in sub-builds had a lot more options. Do they need to also be referenced here at the top-level, or do they get captured automatically somehow? A more detailed description both here and there would be helpful.

The archives section on the subbuilds is more complex because it supports scenarios like subbuilds uploading multiple jar and pom files to a bucket different from the one used for all the other archives and GN+ninja artifacts being generated directly inside the zip_archives folder of the build output.

The global generator use case is simpler because the script have full control about where inside src/out to generate the artifacts and also have the freedom to specify the path relative to the bucket where the artifact will be uploaded to.

I have a pending action item to move the archives inside the global generator configuration to make them simpler to understand and potentially remove complexity required during the migration.

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

chinmaygarde · 2023-05-11T20:23:15Z

Seems like this is still WIP. Please ping @zanderso once this is ready for re-review.

chinmaygarde · 2023-05-25T20:18:41Z

@zanderso @godofredoc Perhaps we can land just the parts that are complete?

godofredoc · 2023-05-25T20:30:41Z

@zanderso @godofredoc Perhaps we can land just the parts that are complete?

Let me update all the comments today and send it for a second review before the end of the day.

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

godofredoc · 2023-05-26T02:33:47Z

All the comments have now been addressed and the suggestions have been applied.

zanderso

Thanks for all the updates. Just a few more comments/questions.

zanderso · 2023-05-26T15:47:27Z

ci/builders/README.md


 ## USAGE EXAMPLES

-Engine builds will be translated one to one using the build definition language with one file per build. The build definition files will be stored in the [flutter/engine/ci/builders](https://cs.opensource.google/flutter/engine/+/main:ci/builders/) directory.
+Engine build definition files using the Build Definition Language can be found in the
+[flutter/engine/ci/builders](https://cs.opensource.google/flutter/engine/+/main:ci/builders/) directory.


Suggested change

[flutter/engine/ci/builders](https://cs.opensource.google/flutter/engine/+/main:ci/builders/) directory.

[flutter/engine/ci/builders](https://github.com/flutter/engine/tree/main/ci/builders) directory.

Not sure if it's a GitHub issue, but I'm still seeing the old URL.

ok, I'll give up on pushing commit and update directly from the github ui. It seems like it is ignoring my changes.

ok, updates are available now.

zanderso · 2023-05-26T15:57:51Z

ci/builders/README.md

+* **name:** - used to identify the archive inside CAS.
+* **base\_path:** - the portion of the path to remove from the full path before
+ uploading to its final destination. In the example the above the
+ base\_path **“out/host\_debug/zip\_archives”** will be removed from the


Is there any situation where you would not want to specify a base_path? If not, and it always has to be specified, then why is it a configurable option? Could it be derived from one of the other options?

Yes, the use case is legacy files generated outside of zip_archives folder or files generated with the incorrect folder structure.

zanderso · 2023-05-26T15:58:28Z

ci/builders/README.md

+ include path **"out/host\_debug/zip\_archives/linux-x64/artifacts.zip"**
+ before uploading to GCS, e.g.
+ &lt;bucket&gt;/flutter/&lt;commit>/linux-x64/artifacts.zip.
+* **Type:** - the type of storage to use. Currently only **“GCS”** and


The question is why would you choose one instead of the other. What's a situation where you'd choose CAS? What's a situation where you'd choose GCS?

Updated, using CAS will allow you to inspect the files during development without having to worry about cleanup on impacting production as they are automatically deleted after a few days of no use. GCS is expected for the final artifacts as it is the location the flutter tool will use to download them.

zanderso · 2023-05-26T16:00:58Z

ci/builders/README.md

-Note that to keep the recipes generic they don’t know anything about what the test script is doing and it is the responsibility of the test script to copy the relevant files to the FLUTTER\_LOGS\_DIR directory.
+* **language** - the executable used to run the script, e.g. python3, bash.
+In general any executable found in the path can be used as language. The
+default value for this property is `bash`.


Even on Windows?

Taking that comment back, default is empty which means script will be treated as executable with the assumption that already has the right permissions.

Bash can be used on all the platforms the caveat is the use of paths in the windows os wich will require something like /c/myfile.exe instead of c:/myfile.exe.

Can you add that detail to the docs, please?

zanderso · 2023-05-26T17:14:46Z

ci/builders/README.md


-The test scripts will run in a deferred context (failing the step only after logs have been uploaded). The tester and builder recipes provide an environment variable called FLUTTER\_LOGS\_DIR pointing a temporary directory where the test runner can place any logs|artifacts needed to debug issues. At the end of the test execution the content of FLUTTER\_LOGS\_DIR will be uploaded to Google Cloud Storage before signaling the pass | fail test state.
+The test scripts will run in a deferred context (failing the step only after
+logs have been uploaded). Tester and builder recipes provide an environment


Might be good to note here to avoid confusion.

ci/builders/README.md

godofredoc · 2023-05-31T19:54:43Z

Thanks for all the updates. Just a few more comments/questions.

Updated!

ricardoamador · 2023-06-01T17:41:44Z

ci/builders/README.md

+sub-builds to generate artifacts.
+* **Builder** - a combination of configuration, recipes and a given commit to
+build and test artifacts.
+* **Build** - a builder running with specific properties, repository and


So these two (builder and build) seem to be heavily overloaded. Build is specified a single top level build that displays in the flutter dashboard and is expressed as a single file containing multiple sub builds.

A builder is also referred to as the runner of this top level build and also a runner of a sub build, which we may want to specify somewhere in here. So builder refers to two different parts of an engine_v2 build.

ricardoamador · 2023-06-01T17:43:46Z

ci/builders/README.md


-```
+The following is a sample build configuration referencing
+[android\_aot\_engine.json](https://github.com/flutter/engine/blob/main/ci/builders/mac_android_aot_engine.json)


nit: if we want to be explicitly clear we should note that you need to omit the .json extension here as the recipe knows that this is expected to be json.

ricardoamador · 2023-06-01T17:45:45Z

ci/builders/README.md


 The following is the high level structure of the build component:

-```
+```yaml


nit: json not yaml.

ricardoamador · 2023-06-01T17:46:14Z

ci/builders/README.md

+is required `cas_archive": false,` needs to be added to the
+configuration.
+
+```yaml


nit: json not yaml.

ricardoamador · 2023-06-01T17:52:26Z

ci/builders/README.md

-The build definition language will force all the artifacts to be generated with GN+Ninja. This will require a carefully designed strategy to replace the artifacts with minimal disruption.
+Global tests follow the same format as build tests but defined at the top
+level. The main difference from local tests is that global tests have access
+to the sub-build artifacts.


nit: consider rephrasing 'to the sub-build artifacts.' as 'to all sub-build artifacts.'

Now that I think about it, what would be the purpose of running the tests locally when they could just be specified as global? I guess if you do not need all the artifacts?

ricardoamador

LGTM

zanderso

Not sure if something is going wrong with GitHub, but it seems like some of the updates are missing.

zanderso · 2023-06-01T17:55:35Z

ci/builders/README.md


 ## USAGE EXAMPLES

-Engine builds will be translated one to one using the build definition language with one file per build. The build definition files will be stored in the [flutter/engine/ci/builders](https://cs.opensource.google/flutter/engine/+/main:ci/builders/) directory.
+Engine build definition files using the Build Definition Language can be found in the
+[flutter/engine/ci/builders](https://cs.opensource.google/flutter/engine/+/main:ci/builders/) directory.


Not sure if it's a GitHub issue, but I'm still seeing the old URL.

ci/builders/README.md

zanderso · 2023-06-02T16:25:02Z

ci/builders/README.md

+ include path **"out/host\_debug/zip\_archives/linux-x64/artifacts.zip"**
+ before uploading to GCS, e.g.
+ &lt;bucket&gt;/flutter/&lt;commit>/linux-x64/artifacts.zip.
+* **Type:** - the type of storage to use. Currently only **“GCS”** and


Sorry, I don't see the update. Not sure if I'm just having problems with GitHub.

zanderso · 2023-06-02T16:26:56Z

ci/builders/README.md

-Note that to keep the recipes generic they don’t know anything about what the test script is doing and it is the responsibility of the test script to copy the relevant files to the FLUTTER\_LOGS\_DIR directory.
+* **language** - the executable used to run the script, e.g. python3, bash.
+In general any executable found in the path can be used as language. The
+default value for this property is `bash`.


Can you add that detail to the docs, please?

ricardoamador

I think this needs more updating as there are fields that need to be included in this doc to better flush out the parts of each build.

The doc needs to be updated.

ricardoamador

Approving since this follow up issue has been created to address a more detailed dive into each of the usable objects: flutter/flutter#128125

…128147) flutter/engine@b4250ac...f3f6a02 2023-06-02 jason-simmons@users.noreply.github.com Wrap concurrent message loop tasks in an autorelease pool on iOS/Mac platforms (flutter/engine#42459) 2023-06-02 godofredoc@google.com Update engine build config docs. (flutter/engine#41468) 2023-06-02 43054281+camsim99@users.noreply.github.com [Android] Bump unit test robolectric version to 4.10.3 (flutter/engine#42454) 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 jonahwilliams@google.com,rmistry@google.com,zra@google.com 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

Update engine build config docs.

9e56abc

* Removed sections relevan only to a design doc. * Remove deprecated properties. * Add docs about default properites. * Rephrased some sections.

godofredoc requested a review from zanderso April 25, 2023 00:39

zanderso reviewed May 3, 2023

View reviewed changes

godofredoc and others added 13 commits May 4, 2023 15:51

Update ci/builders/README.md

f583267

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

c8bcb86

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

192a38f

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

37eb318

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

4fdf13c

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

3022912

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

c2a3b59

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

bbe7cda

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

4c9e231

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

5bc7d4d

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

c061ac9

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

3841c26

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

9537a05

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label May 18, 2023

godofredoc and others added 10 commits May 25, 2023 17:15

Apply suggestions from code review

1cdafb6

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

d7d9a88

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

b307996

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

059976e

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

7451c8a

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

5997e67

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

300f645

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

94834ac

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Update ci/builders/README.md

6e872e5

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Apply comments and update some sections.

1a2bdb1

godofredoc removed the Work in progress (WIP) Not ready (yet) for review! label May 26, 2023

Merge branch 'main' into update_docs

ce889fc

zanderso reviewed May 26, 2023

View reviewed changes

keyonghan reviewed May 31, 2023

View reviewed changes

ci/builders/README.md Outdated Show resolved Hide resolved

Merge branch 'main' into update_docs

e025222

ricardoamador reviewed Jun 1, 2023

View reviewed changes

ci/builders/README.md

The following is the high level structure of the build component:

```

```yaml

Copy link

Contributor

ricardoamador Jun 1, 2023

Choose a reason for hiding this comment

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

nit: json not yaml.

ricardoamador reviewed Jun 1, 2023

View reviewed changes

ricardoamador previously approved these changes Jun 1, 2023

View reviewed changes

zanderso reviewed Jun 2, 2023

View reviewed changes

ricardoamador reviewed Jun 2, 2023

View reviewed changes

ricardoamador self-requested a review June 2, 2023 16:31

ricardoamador approved these changes Jun 2, 2023

View reviewed changes

Apply comments.

9aa4abc

zanderso approved these changes Jun 2, 2023

View reviewed changes

godofredoc merged commit e39bd7c into flutter:main Jun 2, 2023
29 checks passed

godofredoc deleted the update_docs branch June 2, 2023 19:58

engine-flutter-autoroll mentioned this pull request Jun 2, 2023

Roll Flutter Engine from b4250acbf3ca to f3f6a020595d (3 revisions) flutter/flutter#128147

Merged

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

e39bd7ca1 Update engine build config docs. (flutter/engine#41468)

cde323f

	[flutter/engine/ci/builders](https://cs.opensource.google/flutter/engine/+/main:ci/builders/) directory.
	[flutter/engine/ci/builders](https://github.com/flutter/engine/tree/main/ci/builders) directory.


		```yaml
		"archives": [

Update engine build config docs. #41468

Update engine build config docs. #41468

Conversation

godofredoc commented Apr 25, 2023

Pre-launch Checklist

zanderso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde commented May 11, 2023

chinmaygarde commented May 25, 2023

godofredoc commented May 25, 2023

godofredoc commented May 26, 2023

zanderso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanderso May 26, 2023 • edited by godofredoc

Choose a reason for hiding this comment

zanderso May 26, 2023 • edited by godofredoc

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

godofredoc commented May 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardoamador left a comment

Choose a reason for hiding this comment

zanderso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardoamador left a comment

Choose a reason for hiding this comment

ricardoamador left a comment

Choose a reason for hiding this comment

zanderso May 26, 2023 •

edited by godofredoc

zanderso May 26, 2023 •

edited by godofredoc