tree 358dcf767a8045102689dfd9f5110e6a9860de62
parent 3a98b7d0b2200ebb119cf662f0077bd0c0c95d0f
author ulfjack <ulfjack@google.com> 1498465789 +0200
committer Marcel Hlopko <hlopko@google.com> 1498495200 +0200

BlazeCommandDispatcher: all options parsing and editing in one place

The invocation policy must be the last step in determining the active options
for the current command. Take precautions to prevent accidental options editing
of options after the policy is applied by declaring the options as an
OptionsProvider (interface has no mutating methods); technically, this could
be circumvented by casting to OptionsParser, but it's at least safer than
before.

As part of this, I'm moving the BlazeCommand.editOptions call to before the
invocation policy, and also making minor changes to the CommandEnvironment.
Commands that expect to have the last word on options could therefore see
options changes after this commit.

There are three commands that override editOptions:
1. TestCommand
Current behavior: if test_output=streamed is set, also sets
--test_sharding_strategy=disabled and --test_strategy=exclusive.

Overriding --test_sharding_strategy is not a concern; in fact, maybe we
shouldn't be setting that in the first place, since it can cause tests to
timeout (timeout is usually applied per shard, so a 10-way sharded test will
very likely timeout). If you override the test_strategy to local or standalone,
then you may get interleaved stdout / stderr output from tests that run in
parallel - a bit surprising, but no showstopper. If you override the
test_strategy to remote, you won't get streamed output, because no remote
strategy currently supports that. You may get interleaved output, if multiple
tests finish very close to each other.

There are no correctness concerns, it's just a slightly worse user experience.
The code is safe in all cases, AFAICT.

We could consider changing streamed to not require serialized execution, and
instead use something like a lock - the first test to produce output gets
streamed, and all others get delayed until the lock is released, but could
still execute concurrently. However, it's unlikely that that's the desired
behavior for most users.

2. CoverageCommand
Current behavior: sets --collect_code_coverage, increases default
--test_timeout.

bazel coverage --nocollect_code_coverage is effectively bazel test. You can
actually now run bazel test --collect_code_coverage --test_timeout and it will
behave exactly the same way as bazel coverage. It's mostly a convenience thing.

Note that CoverageCommand inherits TestCommand, so the case above also applies.

3. MobileInstallCommand
Current behavior: sets --aspects, and --output_groups.

If you override the aspect or output_groups, then the command will fail or run
an old binary from a previous build. Not what you'd expect, but no correctness
concerns.

Summary:
IMO, the impact on the existing commands is minor. The advantage of this change
is that it's more secure, and gives us an escape hatch if a command ever
overrides options in a way that turns out to be wrong or broken in some way.

RELNOTES: None.
PiperOrigin-RevId: 160114902