Skip to content
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

Configure remote builds to use EngFlow #11887

Merged
merged 2 commits into from
May 3, 2023
Merged

Conversation

shs96c
Copy link
Member

@shs96c shs96c commented Apr 12, 2023

This allows us to use EngFlow's remote build grid to execute our builds. Once we hook this up into our CI actions, this will lead to significantly faster CI runs.

This allows us to use EngFlow's remote build grid to
execute our builds. Once we hook this up into our CI
actions, this will lead to significantly faster CI
runs.
Copy link
Member

@p0deje p0deje left a comment

Choose a reason for hiding this comment

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

I see there is a whole new common/remote-build directory with many autogenerated files. Is there any way to shrink those? For example, I assumed we don't have to manually configure C++ toolchains because I thought it's built-in to Bazel.

.bazelrc Show resolved Hide resolved
@shs96c
Copy link
Member Author

shs96c commented May 2, 2023

The files are auto-generated by (effectively, if not in reality) running bazel on the remote machine and then copying the generated files into the repo. It's not a great idea to modify them.

We don't have a hermetic C++ toolchain, so we still need to manage that ourselves. Our remote build images have a C++ toolchain installed, so we need to tell Bazel about it. Because the build is remote, we can't figure this stuff out at run time (since we need to know everything before sending the actions to the remote machine), and this is the way to configure it.

@p0deje
Copy link
Member

p0deje commented May 2, 2023

The files are auto-generated by (effectively, if not in reality) running bazel on the remote machine and then copying the generated files into the repo. It's not a great idea to modify them.

Got it. I think as long as it's documented how to re-generate these files should we need - we are fine.

We don't have a hermetic C++ toolchain, so we still need to manage that ourselves. Our remote build images have a C++ toolchain installed, so we need to tell Bazel about it. Because the build is remote, we can't figure this stuff out at run time (since we need to know everything before sending the actions to the remote machine), and this is the way to configure it.

Ok, so Bazel doesn't ship with a hermetic C++ toolchain. Would using something like https://github.com/uber/hermetic_cc_toolchain help us? Probably not now, but eventually.

@shs96c
Copy link
Member Author

shs96c commented May 2, 2023

Got it. I think as long as it's documented how to re-generate these files should we need - we are fine.

We just ran https://github.com/bazelbuild/bazel-toolchains

Ok, so Bazel doesn't ship with a hermetic C++ toolchain. Would using something like https://github.com/uber/hermetic_cc_toolchain help us? Probably not now, but eventually.

I spent an ungodly amount of time attempting to get the zig-cc toolchain working and gave up. If all we wanted to do was support Linux, it'd be easier, but we want to support the matrix of compiling for and on Windows, Mac OS, and Linux. For that we need various sysroots, and the legality of distributing these is unknown. That makes it a really difficult problem to solve. It's also not something that we can solve in this PR :)

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 75.34% and project coverage change: +0.01 🎉

Comparison is base (e79b2f2) 54.87% compared to head (a5c0cad) 54.89%.

❗ Current head a5c0cad differs from pull request most recent head c594b77. Consider uploading reports for the commit c594b77 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #11887      +/-   ##
==========================================
+ Coverage   54.87%   54.89%   +0.01%     
==========================================
  Files          85       86       +1     
  Lines        5698     5733      +35     
  Branches      230      233       +3     
==========================================
+ Hits         3127     3147      +20     
- Misses       2341     2353      +12     
- Partials      230      233       +3     
Impacted Files Coverage Δ
py/selenium/webdriver/common/proxy.py 17.28% <0.00%> (+0.10%) ⬆️
py/selenium/webdriver/safari/webdriver.py 57.14% <0.00%> (+0.67%) ⬆️
py/selenium/webdriver/webkitgtk/webdriver.py 27.27% <25.00%> (+0.60%) ⬆️
py/selenium/webdriver/common/selenium_manager.py 69.84% <41.17%> (-18.09%) ⬇️
py/selenium/webdriver/firefox/webdriver.py 53.41% <50.00%> (-0.05%) ⬇️
py/selenium/webdriver/wpewebkit/webdriver.py 34.48% <50.00%> (+2.48%) ⬆️
py/selenium/__init__.py 100.00% <100.00%> (ø)
py/selenium/webdriver/__init__.py 100.00% <100.00%> (ø)
py/selenium/webdriver/chrome/webdriver.py 72.91% <100.00%> (+3.86%) ⬆️
py/selenium/webdriver/common/driver_finder.py 100.00% <100.00%> (ø)
... and 4 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@p0deje p0deje merged commit d530584 into SeleniumHQ:trunk May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants