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

[Cherrypicks for 4.2.0] Cherrypick two fixes #13776

Conversation

larsrc-google
Copy link
Contributor

Fixes the default worker JSON protocol handler not allowing unknown fields, and a crash bug (#13240)

@google-cla
Copy link

google-cla bot commented Jul 30, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Jul 30, 2021
larsrc-google and others added 11 commits July 30, 2021 18:17
… branch of dynamic execution."

This reverts commit 5b95d91.
…after all strategies have been tried, and improve messages around it."

This reverts commit b2231c5.
…ategy flag is passed.

RELNOTES: n/a
PiperOrigin-RevId: 344274807
Allow `DynamicExecutionModule` to specify an extra spawn to be ran in the local
branch. Add support in `DynamicSpawnStrategy` for running the extra spawn when
it is provided.

PiperOrigin-RevId: 344826682
…ncel each other.

RELNOTES: None.
PiperOrigin-RevId: 352596000
…l strategies have been tried, and improve messages around it.

RELNOTES: None.
PiperOrigin-RevId: 371873129
…of dynamic execution.

Under --experimental_local_lockfree_output, the local branch may succeed and set the future before cancelling the remote branch. If the remote branch succeeded after that but before the local listener got going, it would falsely think the local was cancelled, which would occasionally lead to strange errors.

RELNOTES: None.
PiperOrigin-RevId: 373347494
…ution fail to accept (that is, refuse to even take) the action given, the whole action fails. Instead of seeing whether the other branch can run and the action that that it succeeded.

NOTE: This is _not_ about the local/remote execution branch trying to run it and failing. Obviously that should fail the whole dynamic execution. This is about none of the strategies of one branch even stating that they can run it (based on what they return from `canExec`)

If the local execution branch requires a specific architecture/OS for some action, and users' bazel is being run a machine that isn't that combination (assuming the local strategy given is already setup to return `false` in `canExec` in this case).

Previously the whole execution of the dynamic execution would fail without even trying to see if remote execution succeeded. Now it will gracefully fallback to just waiting on remote strategy.

To conditionally use workers for supported actions but still simultaneously kick off a dynamic run, the options`--stragegy=TaskMnemonic=dynamic --dynamic_local_strategy=TaskMnemonic=worker --dynamic_remote_strategy=TaskMnemonic=remote` can be given.
Then any action with with the mnemonic `TaskMnemonic` that can't support `worker` execution (xor `remote` execution) will wait for the other execution branch to complete. If neither set of strategies can run the action, then the task fails.

Previously, this would have failed if, for example, the `worker` strategy cannot handle the action, even if `remote` could have.

This at first glance seems silly as if you know TaskMnemonic doesn't have a worker enabled implementation, why specify `worker` as the local strategy?
But keep in mind any action can declare most any mnemonic. In this example, say that first rule doing a TaskMnemonic is worker enabled, so you add the flags. But then someone makes a bazel/starlark rule with the same mnemonic but different implementation (for example, a "mimic" rule), and that new one doesn't support worker. Then this becomes a case of "partial" worker support for a mnemonic.

RELNOTES: If all strategies of one branch (the local or remote execution branch) of the `dynamic` strategy fail to even accept (via the response they give from `canExec`) the action, `dynamic` will now try to see if the other branch can accept it. (Trying to run it and it failing will still cause a failure if it was the first result, this is about strategies claiming they can't even try the action)
PiperOrigin-RevId: 374265582
Races in `stopBranch` caused the losing branch to throw `DynamicInterruptedException`, which could then get to that branch's listener before the winning branch actually cancelled.

RELNOTES: None.
PiperOrigin-RevId: 383126912
…elbuild#13240.

This code assumed that `registerSpawnStrategies` would be always be called before `afterCommand`, but that's not the case if the build was interrupted or failed before the execution phase.

RELNOTES: None.
PiperOrigin-RevId: 379937925
RELNOTES: None.
PiperOrigin-RevId: 381428739
@google-cla
Copy link

google-cla bot commented Jul 30, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants