-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Add multiplex sandboxing support to JavaBuilder #21370
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
Conversation
|
I tried adding a test based on @wilwell Would you consider it acceptable to loosen the latter check? |
6639f93 to
b69afd4
Compare
cushon
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.
Thanks! The JavaBuilder changes look great to me.
@wilwell do you want to review the src/main/java/com/google/devtools/build/lib/worker/ part?
src/main/java/com/google/devtools/build/lib/worker/WorkerMultiplexer.java
Show resolved
Hide resolved
wilwell
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.
Thanks for your contribution!
|
@wilwell Do you have an opinion regarding #21370 (comment)? |
|
@fmeum , could you explain again #21370 (comment) please? If I understood correctly you have an issue with flag And what is you suggestion? You want to change the behaviour of supportsMultiplexSandboxing? |
|
@wilwell Yes, that would be one way of achieving my goal: I would like to see some way to enable sandbox multiplexing for Java actions that 1) doesn't require building a custom Bazel binary and 2) is considered experimental enough that you are willing to accept it before committing to fully support the feature. An alternative could be to expose this on |
|
@fmeum could you specify a little bit more details. What do you pass here? Is it In general I think, that instead of fixing the value cc @meisterT |
|
Ahh, sorry I understood your alternative. Yes, let's try add toolchain flag My thoughts about alternatives
@fmeum Am I right, that if you add |
90c937c to
e2006c7
Compare
|
@wilwell Done, please review again. I also added a test. |
wilwell
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.
Looks good to me.
|
@wilwell Could you add the |
|
@bazel-io fork 7.1.0 |
|
@sgowroji Is this already in the process of being imported? |
There are some internal references to the modified code that require small updates, but it's making progress. |
This does not include adding support to Java actions yet. Work towards bazelbuild#21091 Closes bazelbuild#21370. PiperOrigin-RevId: 613231195 Change-Id: I87e76d2cebed3075a1eedfd8445b910ebd7604b9
|
The changes in this PR have been included in Bazel 7.1.0 RC2. Please test out the release candidate and report any issues as soon as possible. |
This does not include adding support to Java actions yet.
Work towards #21091