Skip to content

Conversation

@fmeum
Copy link
Collaborator

@fmeum fmeum commented Feb 15, 2024

This does not include adding support to Java actions yet.

Work towards #21091

@fmeum fmeum requested a review from a team as a code owner February 15, 2024 15:59
@github-actions github-actions bot added team-Rules-Java Issues for Java rules team-Local-Exec Issues and PRs for the Execution (Local) team awaiting-review PR is awaiting review from an assigned reviewer labels Feb 15, 2024
@fmeum fmeum requested review from cushon and removed request for a team February 15, 2024 15:59
@fmeum
Copy link
Collaborator Author

fmeum commented Feb 15, 2024

@fmeum fmeum force-pushed the 21091-multiplex-sandbox branch from 6639f93 to b69afd4 Compare February 15, 2024 16:09
Copy link
Contributor

@cushon cushon left a 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?

@cushon cushon requested a review from wilwell February 15, 2024 16:28
@fmeum fmeum requested a review from wilwell February 16, 2024 11:51
Copy link

@wilwell wilwell left a 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!

@fmeum
Copy link
Collaborator Author

fmeum commented Feb 16, 2024

@wilwell Do you have an opinion regarding #21370 (comment)?

@wilwell
Copy link

wilwell commented Feb 21, 2024

@fmeum , could you explain again #21370 (comment) please?

If I understood correctly you have an issue with flag --modify_execution_info because it only adds the flag key and not flag value (set it to "" by default).

And what is you suggestion? You want to change the behaviour of supportsMultiplexSandboxing?

@fmeum
Copy link
Collaborator Author

fmeum commented Feb 21, 2024

@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 java_toolchain as in Lars' CL, but without enabling it for the default toolchains. If you see a different way that you like better, I'm of course open to that as well.

@wilwell
Copy link

wilwell commented Feb 27, 2024

@fmeum could you specify a little bit more details. What do you pass here? Is it modify_execution_info=Javac=supports-multiplex-sandboxing?

In general I think, that instead of fixing the value supports-multiplex-sandboxing flag we need to fix the behaviour of --modify_execution_info flag to be able to pass the key-value pair there.

cc @meisterT

@wilwell
Copy link

wilwell commented Feb 27, 2024

Ahh, sorry I understood your alternative. Yes, let's try add toolchain flag javac_supports_worker_multiplex_sandboxing as Lars did in his CL.

My thoughts about alternatives

  1. Change supports-multiplex-sandboxing check to compare with "". It makes check on worker flags inconsistent and could confuse engineers in future.
  2. Change the behaviour of modify_execution_info flag to accept key-value inputs. Its more reliable solution, but it need the large change in modify_execution_info flag and checks that it will not ruin other builds, which is more complicated.
  3. Create javac_supports_worker_multiplex_sandboxing toolchain flag and set it false by default. With this solution engineers who want to interact multiplex Java workers could do it by one flag set. It doesn't require large change in code. The only weakness which I see is inefficient use of remote cache, because the toolchain will be different, if I remember correctly.

@fmeum Am I right, that if you add javac_supports_worker_multiplex_sandboxing flag with default false value one could override this value for their build in their bazelrc file? What is this change?

@wilwell wilwell self-requested a review February 27, 2024 13:31
@fmeum fmeum force-pushed the 21091-multiplex-sandbox branch from 90c937c to e2006c7 Compare February 27, 2024 20:43
@fmeum fmeum requested a review from lberki as a code owner February 27, 2024 20:43
@fmeum fmeum removed the request for review from lberki February 27, 2024 20:43
@fmeum fmeum requested a review from cushon February 28, 2024 11:49
@fmeum
Copy link
Collaborator Author

fmeum commented Feb 28, 2024

@wilwell Done, please review again. I also added a test.

Copy link

@wilwell wilwell left a 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.

@fmeum fmeum requested a review from wilwell February 28, 2024 12:39
@fmeum
Copy link
Collaborator Author

fmeum commented Feb 28, 2024

@wilwell Could you add the awaiting-PR-merge label in addition to your approval?

@wilwell wilwell added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Feb 28, 2024
@fmeum fmeum removed the awaiting-review PR is awaiting review from an assigned reviewer label Feb 28, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Feb 28, 2024

@bazel-io fork 7.1.0

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 5, 2024

@sgowroji Is this already in the process of being imported?

@cushon
Copy link
Contributor

cushon commented Mar 5, 2024

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.

@copybara-service copybara-service bot closed this in 05af69b Mar 6, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Mar 6, 2024
@fmeum fmeum deleted the 21091-multiplex-sandbox branch March 6, 2024 22:41
fmeum added a commit to fmeum/bazel that referenced this pull request Mar 6, 2024
This does not include adding support to Java actions yet.

Work towards bazelbuild#21091

Closes bazelbuild#21370.

PiperOrigin-RevId: 613231195
Change-Id: I87e76d2cebed3075a1eedfd8445b910ebd7604b9
github-merge-queue bot pushed a commit that referenced this pull request Mar 6, 2024
This does not include adding support to Java actions yet.

Work towards #21091

Closes #21370.

PiperOrigin-RevId: 613231195
Change-Id: I87e76d2cebed3075a1eedfd8445b910ebd7604b9

Closes #21518
@iancha1992
Copy link
Member

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.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=7.1.0rc2. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team-Local-Exec Issues and PRs for the Execution (Local) team team-Rules-Java Issues for Java rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants