Add a test for platform(exec_properties={"no-remote-exec": "true"})

[EdSchouten]

Apparently platform(exec_properties) propagates to ctx.actions.run(execution_requirements). This means that it is possible to mark entire platforms as "no-remote-exec".

Let's add some testing coverage for this, to make sure it continues to work going forward.

[katre]

I absolutely understand and support the desire behind this, but adding a new exec-related attribute to platform just makes future problems with updating the API. In specific, this because removing legacy attributes is complicated and largely impossible: witness the fact that remote_execution_properties still exists.

I see that Spawns.mayBeExecutedRemotely is checking for the "no-remote-exec" in the execution info, is that correct?

Theoretically, we're already copying data from platform.execution_properties to the execution info, but the logic is complicated and so it might not be mapping properly. However, I think that we should be able to support the following:

platform(
    name = "not_remote",
    exec_properties = {
        "no-remote-exec": "true",
    },
)

While admittedly not as clear as your new attribute, and definitely harder to discover and use, this is much better for our future plans for platforms and exec platforms (in addition to Platform-Scoped Spawn Strategies, see the original Execution Platforms vs Strategies, and the discussions in #22000 and the discussion about Selectively Enabling Execution Platforms).

Can you test out my suggestion, to see if it works? If it doesn't, and you can adapt your test from this PR, I can debug to figure out why it doesn't.

[katre]

As a side note, I am a bit surprised by the test failure you're seeing in CI, and I don't think it's related to your PR specifically.

[EdSchouten]

I see that Spawns.mayBeExecutedRemotely is checking for the "no-remote-exec" in the execution info, is that correct?

It checks for no-remote-exec in execution_requirements.

Theoretically, we're already copying data from platform.execution_properties to the execution info, but the logic is complicated and so it might not be mapping properly. However, I think that we should be able to support the following:

platform(
    name = "not_remote",
    exec_properties = {
        "no-remote-exec": "true",
    },
)

exec_properties is different from execution_requirements, right? The former contains key/values that get passed along to REv2's platform properties (e.g., "OSFamily" = "linux"). The latter contains thinks like no-remote, no-remote-exec, no-cache, etc.. Even though they are similarly named, they are in no way related/interchangeable to each other, right?

[EdSchouten]

exec_properties is different from execution_requirements, right? The former contains key/values that get passed along to REv2's platform properties (e.g., "OSFamily" = "linux"). The latter contains thinks like no-remote, no-remote-exec, no-cache, etc.. Even though they are similarly named, they are in no way related/interchangeable to each other, right?

Woah, interesting! It looks like platform(exec_properties) does propagate to ctx.actions.run(execution_requirements). How unexpected!

I've just updated this PR to remove any code changes I made. I did leave the test case behind. Maybe it's good to merge that, to ensure that this behaviour is guaranteed going forward.

[katre]

The trifecta of "exec properties", "exec info", and "exec requirements" are very confusing, and IMO we should collapse them into one set.

To answer your question: an exec property with a "true-ish" value (like "1" or "TRUE" or "yes") has the key added to the exec info and requirements. I wanted to convert key/values like "remote-exec": "no" to "no-remote-exec", but that was deemed too confusing and so we dropped it.

[katre]

Very happy to add this test and ensure the behavior stays. Thanks for writing this, and working through the options with me!

[katre]

Actually, one more change: can you edit the title and description, before I put in the request to merge this?

[EdSchouten]

Actually, one more change: can you edit the title and description, before I put in the request to merge this?

Done. Thanks!

[coeuvre]

Thanks!