Per-target exec_properties overwrites (not merges with) --remote_default_exec_properties

[jmillikin-stripe]

Description of the problem / feature request:

exec_properties is a universal target attribute documented as:

A dictionary of strings that will be added to the exec_properties of a platform selected for this target.

This doesn't seem to be working as expected when combined with --remote_default_exec_properties. Instead of merging, the default exec properties get completely ignored.

The build is not using a platform() rule (the container-image name is too dynamic to easily put in a source file), so I would have expected --remote_default_exec_properties to be used.

What operating system are you running Bazel on?

Linux

What's the output of bazel info release?

release 1.0.0

[brentleyjones]

This recently bit us as well.

[coeuvre]

As described in the doc:

Set the default exec properties to be used as the remote execution platform if an execution platform does not already set exec_properties.

So the current behaviour is correct but I am not sure whether it's expected.

[jmillikin-stripe]

I read that as referring to the platform's exec_properties, not the target's exec_properties.

In other words, if a platform doesn't have exec_properties set, it will receive the value of --remote_default_exec_properties.

However, regardless of the platform's content, I expect the per-target exec_properties to be merged with it in accordance with the documentation I quoted in the first post.

Docs (in Common Definitions):

exec_properties
Dictionary of strings; optional

A dictionary of strings that will be added to the exec_properties of a platform selected for this target. See exec_properties of the platform rule.

If a key is present in both the platform and target-level properties, the value will be taken from the target.

[brentleyjones]

Same, I read " if an execution platform does not already set exec_properties." to mean the platform set it, not a target. Regardless, like @jmillikin-stripe says, I feel the target level one should merge with either.

[coeuvre]

SGTM. I will make the fix into 5.0.