Make --modify_execution_info additive

[sitaktif]

Calling --modify_execution_info multiple times used to result in the last version being used. With this change, options are amended additively.

Fixes #13342

[sitaktif]

I was told @meisterT might be happy to take a look maybe?

[meisterT]

I wonder whether this has the potential to break people?

[sitaktif]

In practice, I'm not sure how likely it is to break people but technically, it is a breaking change.

Do we think it's worth putting that behind an --incompatible flag (making it enabled by default on bazel 6+)?

It would be great to have feedback from users to understand if this change would be incompatible for their use case 🙂

[meisterT]

It may be wise to reach out to https://groups.google.com/g/bazel-discuss (and/or bazel-dev) to find out

[brentleyjones]

This would break a couple usages I know of, but not in a way that can't be recovered from with using the negation ability to remove stuff previously added.

[sitaktif]

I started a thread in bazel-discuss to gather more feedback 👍

[coeuvre]

Just want to point it out, this flag is used internally across many projects. Just making this additive will likely break them. Adding an incompatible flag requires having a plan to migrate existing usages. I don't think we have the capacity to do that now.

[sitaktif]

@coeuvre thanks for sharing.

Could we not settle with a simple migration plan such as "activate it for the next major Bazel version"?

[sitaktif]

The approach above could be taken with either:

• an --incompatible_additive_modify_execution_info_flag flag (switching the default from false to true in Bazel 6)
• no additional flag, keep old behavior until bazel 6.

The latter is simpler, but makes Bazel 6 migration harder, so I personally lean towards the former.

Are there additional complexities I may not be aware of?

[sitaktif]

Updated the PR to add an --incompatible_modify_execution_info_additive_flag option, preserving the current behavior by default.

---

Adding an incompatible flag requires having a plan to migrate existing usages. I don't think we have the capacity to do that now.

@coeuvre to be honest, it doesn't feel like this should be a valid argument when discussing features for an open-source project. Providing a feature flag for the feature make it convenient for projects to keep the existing behavior. The existence of a process to eventually migrate out of it feels like a... second order constraint? Hard to put my finger on it exactly :-)

That said, assuming there is a limited number of values used by the repo, there's a straightforward way to migrate existing usages in a way that doesn't change between non-additive (ie. current) and additive behavior: you can change any --modify-execution-info flags from:

build --modify_execution_info=Foo=+value-one,Bar=+value-two

to:

build --modify_execution_info=.*=-value-one,.*=-value-two,.*=-value-three,Foo=+value-one,Bar=+value-two

or (maybe more legible):

build --modify_execution_info=.*=-value-one,.*=-value-two,.*=-value-three
build --modify_execution_info=Foo=+value-one,Bar=+value-two

This example assumes that only three values are used across the repo. It can work for any number of values, though there is a point at which it might feel overwhelming.

[meisterT]

cc @susinmotion

[brentleyjones]

@bazelbuild/triage this stalled out. Can we get it looked at?

[sitaktif]

I’d be happy to rebase this change or discuss alternative if people want to move forward with it. I still think it would be a natural improvement for Bazel.

By the way, I realise the tone in my last comment wasn't the most appropriate, sorry about that 🙏

[coeuvre]

I am fine moving forward with current approach, i.e. add an incompatible flag and only make --modify_execution_info additive when the flag is set. I had some ideas for the migration but let's postpose it until we are about to flip the incompatible flag.

Please rebase the PR and I will take a proper review, thanks!

[wilwell]

Hi @sitaktif !

Could you please rebase the change for a proper view to finalize the review process?

[sitaktif]

@coeuvre @wilwell thanks, I just submitted the rebased patch.

(The typo in my branch name irritates me every time but it's not worth opening a new PR just for that reason...)

[coeuvre]

We want to pass the original input from the command line to the first argument because it is used for equality check. Maybe join eim.option() with ,? Please also assert the value of option() for the merged modifier in the test cases.

[sitaktif]

Thanks - updated 👍

[coeuvre]

Thanks! I have imported the PR and made some changes on top of it:

1. Renamed the incompatible flag to --incompatible_modify_execution_info_additive.
2. Instead of merging the modifiers, it now iterates though the list to avoid creating transient collection object. It matters because the code is called for reach action.

You can check the updated code here: https://bazel-review.git.corp.google.com/c/bazel/+/249650.

[coeuvre]

@bazel-io fork 7.2.0

[sitaktif]

Thanks for taking the time to review/improve this @coeuvre! 🙏