[ci] Add OS version to action key in CI

[milesdai]

This commit injects the OS version into a paramter that is used as part
of the action key computation. This should prevent artifacts from
different OS's from colliding in the remote cache.

Signed-off-by: Miles Dai milesdai@google.com

Fixes #14695

[milesdai]

Based on the discussion here, I decided to use the --remote_default_platform_properties for this.

[a-will]

Based on the discussion here, I decided to use the --remote_default_platform_properties for this.

Apparently, it's...

DEPRECATED. Please use exec_properties attribute instead.

Oh wait, sorry, this is a different thing. Now I better read again. 😂

But ostensibly, it looks like --remote_default_exec_properties would touch the desired key, whereas this one does the deprecated remote_execution_properties.

[alphan]

DEPRECATED. Please use exec_properties attribute instead.

The message of bazelbuild/bazel@6ec75ab (the commit that adds the new option and the deprecation message) might help.

[milesdai]

Changed to --remote_default_exec_properties - thanks all for the help! @alphan I'm adding you as a reviewer as well just to get some more eyes on the bash.

[milesdai]

The previous run did in fact work -- it's just that the FPGA build took almost 40 minutes. So on the plus side, it turns out we are indeed making good use of the caching. We'll just need this build to go through to re-seed the cache since we have effectively just invalidated everything in there now.

[a-will]

The previous run did in fact work -- it's just that the FPGA build took almost 40 minutes. So on the plus side, it turns out we are indeed making good use of the caching. We'll just need this build to go through to re-seed the cache since we have effectively just invalidated everything in there now.

Do we also need to make sure the opentitan platform has exec_properties defined (i.e. not picking up defaults), so the device-side stuff can still be cached and shared?

(It looks like that would be done in the crt repo, though.)

[alphan]

Thanks @milesdai, just minor nits.

[milesdai]

Do we also need to make sure the opentitan platform has exec_properties defined (i.e. not picking up defaults), so the device-side stuff can still be cached and shared?

(It looks like that would be done in the crt repo, though.)

I haven't familiarized myself with Bazel's platform features yet. If this doesn't affect correctness, should we get this merged first, and look into this later?

[a-will]

Do we also need to make sure the opentitan platform has exec_properties defined (i.e. not picking up defaults), so the device-side stuff can still be cached and shared?
(It looks like that would be done in the crt repo, though.)

I haven't familiarized myself with Bazel's platform features yet. If this doesn't affect correctness, should we get this merged first, and look into this later?

Aye, the comment was mostly a discussion about to-do items after this PR. 🙂

[martin-lueker]

Thanks @a-will for pointing me to this. @milesdai: Is this ready to go in?

[milesdai]

Yes, I think this is good to go

[drewmacrae]

The description calls the use of an incompatible artifact a collision, when really it's just incompatible because of an undocumented dependency on the system, right? Collisions make me think of a crowded cache with too small a key, whereas here the key just doesn't include the information that's sufficient to find an appropriate artifact. (The description can be edited and corrected after the fact, the commit message shouldn't be corrected)

[drewmacrae]

Is there a plan to make the cache available to non-ci consumers like it was before by putting this into .bazelrc somewhere? If not could we document that the chache doesn't support non-ci users? I'd been using it experimentally until July, without enough instrumentation to see performance gains, but if I'm not mistaken, this is going to prevent cache hits.

[milesdai]

Yes this will prevent cache hits from anyone not running Bazel with ci/bazelisk.sh. On our website, we don't actually specify that the remote cache should be used. Rather, a disk cache is recommended.