-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Update engine setup docs. #176750
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
Update engine setup docs. #176750
Conversation
- Move "gclient bootstrap" out of engine/README.md into a section in the "Setting up the Engine development environment" doc. - In the "gclient bootstrap" section, instruct Googlers to follow the RBE setup docs. - Update the "Additional Steps for Web Engine" to reflect the fact that the user's gclient file is copied from a template that contains a commented-out web engine entry. - Link to the "Engine Tool" docs instead of to "Compiling the engine" for the next steps. - In the RBE doc: - Update the gclient instructions to reflect that the gclient file may be copied from rbe.gclient. - Move the "gcloud" section into the "Getting started" parent section. The gcloud setup is required before any of the next steps. - Instead of suggested the "gcloud auth application-default login" command in case of an error, directly instruct the user to run this command. It's required for things to work. - Add a "Too many open files" section to the Troubleshooting parent section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request does a great job of centralizing and clarifying the engine setup documentation. The changes make the instructions easier to follow, especially for new contributors. I have a couple of minor suggestions to further improve the documentation's clarity and long-term maintainability.
|
|
||
| ### Add `et` to `PATH` | ||
|
|
||
| The "Engine Tool" called `et` is useful when working with the engine. It is located in the [`flutter/engine/src/flutter/bin`](https://github.com/flutter/flutter/tree/0c3359df8c8342c8907316488b1404a216f215b6/engine/src/flutter/bin) directory. Add this to your `$PATH` in your `.rc` file: e.g. on UNIX, using `export PATH=/path/to/flutter/engine/src/flutter/bin:$PATH`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link to the et tool's directory contains a hardcoded commit hash. This can become outdated and point to a non-existent path if the repository history is changed. It's better to link to the main branch to ensure the link always points to the latest version of the directory.
| The "Engine Tool" called `et` is useful when working with the engine. It is located in the [`flutter/engine/src/flutter/bin`](https://github.com/flutter/flutter/tree/0c3359df8c8342c8907316488b1404a216f215b6/engine/src/flutter/bin) directory. Add this to your `$PATH` in your `.rc` file: e.g. on UNIX, using `export PATH=/path/to/flutter/engine/src/flutter/bin:$PATH`. | |
| The "Engine Tool" called `et` is useful when working with the engine. It is located in the [`flutter/engine/src/flutter/bin`](https://github.com/flutter/flutter/tree/main/engine/src/flutter/bin) directory. Add this to your `$PATH` in your `.rc` file: e.g. on UNIX, using `export PATH=/path/to/flutter/engine/src/flutter/bin:$PATH`. |
| Increase the maximum number of open files on your machine with the instructions | ||
| [here](go/building-chrome-mac#configure-your-mac-for-remote-execution). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This go/ link is internal to Google and will appear as a broken link to the public on GitHub. While this document is targeted at Googlers, it's still in a public repository. For better accessibility and to avoid confusion, consider either inlining the instructions if they are simple, or replacing the go/ link with a publicly accessible one if available.
For example, you could replace the link with the actual instructions:
Increase the maximum number of open files on your machine. For example, you can add `ulimit -n 10240` to your shell's startup script (e.g., `~/.zshenv` or `~/.bash_profile`) and start a new shell session.| Increase the maximum number of open files on your machine with the instructions | |
| [here](go/building-chrome-mac#configure-your-mac-for-remote-execution). | |
| Increase the maximum number of open files on your machine. For example, you can add `ulimit -n 10240` to your shell's startup script (e.g., `~/.zshenv` or `~/.bash_profile`) and start a new shell session. |
gaaclarke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
| In the engine repo, all RBE builds must be initiated through the `et` tool whose | ||
| entrypoint is the script `//flutter/bin/et`. This is so that the local RBE | ||
| proxy is correctly initialized and shut down around invocations of `ninja`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that's true? I think ninja directly will use RBE too, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph is unchanged by this commit. It's just highlighted in this diff because I moved some lines around in the file.
It likely does have some incorrect information though:
- There is no more engine repo, since it's been moved inside the flutter repo.
- As far as I can tell, the
ettool is located in//engine/src/flutter/bin, not//flutter/bin/et.
For your question about whether et is required for RBE builds, or whether using ninja directly uses RBE: I don't know enough about how things work to have an informed take on this. Can you suggest a more correct way to reword this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no action required: I don't think that what is written is true. If it already was there and this is just moved I'm fine with just assuming it's true. It's not worth further investigation.
- Move "gclient bootstrap" out of engine/README.md into a section in the "Setting up the Engine development environment" doc. - In the "gclient bootstrap" section, instruct Googlers to follow the RBE setup docs. - Update the "Additional Steps for Web Engine" to reflect the fact that the user's gclient file is copied from a template that contains a commented-out web engine entry. - Link to the "Engine Tool" docs instead of to "Compiling the engine" for the next steps. - In the RBE doc: - Update the gclient instructions to reflect that the gclient file may be copied from rbe.gclient. - Move the "gcloud" section into the "Getting started" parent section. The gcloud setup is required before any of the next steps. - Instead of suggested the "gcloud auth application-default login" command in case of an error, directly instruct the user to run this command. It's required for things to work. - Add a "Too many open files" section to the Troubleshooting parent section. <!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> *Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.* *List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.* *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## 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], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.