Rework how bazel stamping work in the repository

[pamaury]

This PR reworks completely how stamping works in the repository:

• It introduces stamp.bzl which is a module to handle stamping. Notably, it allows rules to add stamp parameters
  that behaves like the usual bazel conversion (0 means no stamping, 1 is always stamping, -1 follows the command line).
• It fixes several python tool and bazel rule to use this module so that they only stamp if required to do so.
• It removes some targets that were always stamping and have now become useless

What does this change do:
With this PR, there are two main changes:

• Certain headers generated by reggen (autogen_hjson_header, the tock files) do not contain stamping information anymore by default. If one requires stamping, see below.
• The topgen code has been updated but it is not called direcly by bazel (it is called by make) so the default behaviour remains the same and this does not affect bazel caching anyway.
• Opentitantool is not stamped anymore by default so the version command will print:

{
  version: null,
  revision: null,
  clean: null
}

The build timestamp is removed also since it adds no useful information.

Open question:
The chip_info section of the ROM still includes the git sha by default. We could change this behaviour by setting stamp = -1 in the definition of autogen_chip_info_src but the rom_chip_info.py script will error out because it always exactly a valid SHA to embed. This is problematic though because it will prevent proper caching of the ROM and the tests. I think we should not stamp by default but I see at least two options to do that:

• make the rom_chip_info.py script use a default value if none is provided: this is easy but implicit/hidden and easy to forget, I am not a fan of this idea.
• modify the stamping code so that we explicitely pass a default version file when stamping is disabled and we store a default value of the sha in the ROM when stamping is disabled. This produces the same effect but with the added benefit that this is a lot more explicit. I envision something like this:

autogen_chip_info_src = rule(
    implementation = _chip_info_src,
    attrs = {
        "_tool": attr.label(
            default = "//util:rom_chip_info",
            executable = True,
            cfg = "exec",
        ),
    } | stamp_attr(-1, "//rules:stamp_flag", "//path/to/default/version/file/for/rom_chip_info"),

Summary
I believe thoses changes are acceptable since by default (i.e. in CI or locally), embedding this information does not really make sense. However, when doing a release, we definitely want to have this information. We need to make sure that various release flows add the --stamp parameters to recover the previous behaviour. Alternatively, we could make stamping the default and add --nostamp in CI.

[jwnrt]

These changes LGTM but I'm not sure I understand what happens when stamping is disabled. Will parse_version_file not find the version file and return an empty dictionary? How does that translate to the data that gets inserted into files, i.e. what will this Tock comment look like?

https://github.com/lowRISC/opentitan/blob/master/util/reggen/gen_tock.py#L398

[pamaury]

Yes parse_version_file will return an empty dictionary and it will print something like:

// Generated register constants for rv_plic.
// Built for <unknown>
// https://github.com/lowRISC/opentitan/tree/<unknown>
// Tree status: <unknown>
// Build date: <something>

I think this is a bit ugly so my plan is to go through the python code and make them not generate those string at all if there is no information to display. I think this is more reasonable than displaying the above, what do you think?

[jwnrt]

Yes, I agree this sounds like a good plan.

The one case you may have issues with is that year that gets inserted into the Tock copyright header. I think the correct year for that header should be the year that the template for the generated file was written by a human anyway and it shouldn't be updated every time the file is generated.

Anyway, maybe that's not an issue and you can just leave the year out entirely.

[pamaury]

Yes, in the current code (not yet pushed), I am leaving out the year entirely. Anyway, most files in our repository do not include a year in the copyright.

[nbdd0121]

I think I'd prefer the current approach where a version file is produced to them, instead of having all these tools having to parse volatile-status.txt.

I think having an extra step in the middle (like we currently do), have an advantage that if we need stamping, we can still have some cache hit if some bits of volatile-status.txt changed, but the needed bits happen to stay the same.

[pamaury]

At the moment though, we have a mix of tools using those "derived" files and some other parsing volatile-status.txt (or a copy thereof). This also means that we would have to use "fake" values when stamping is disabled which I don't find to be particularly useful.
Also, some tools like reggen use at least four different pieces of information so that would require to pass four different files on the command-line which I am not sure is much better than passing a single file with a known format?
What I am proposing is that all tools use the same format which is parsed by the same code contained in a python library so there is no code duplication.

[pamaury]

I have done some fixes to topgen and also remove the use of the timestamp entire: I agree with @nbdd0121 that the generation time and date seems irrelevant, what matters is the git revision.

[jwnrt]

LGTM modulo the style error, nice work!

[pamaury]

I had to make some changes so that the generator emits exactly the same code as previously (otherwise I would have to regenerate some files for no good reason). I also reverted partially the change to rom_info so that it always stamps (see updated text at the top).

[pamaury]

I have updated the PR to disable stamping in opentitantool by default as well.

[nbdd0121]

This feels like a hack. Could you make it so that if stamping is disabled, the stamp-env is not given at all? Then you can just use std::option_env!(...).

[pamaury]

My understanding is that this is not possible with the way rules_rust handles rustc_env_file: stamping is handled by replacing {BUILD_SCM_REVISION} and others literally in the env file. If stamping is disabled, the replacement does not occur. But the content of the stamp-env.txt file does not depend on whether stamping is enabled or not. This, admitedly, make the way stamping is handled by rustc not so useful.
We could add custom feature to our rules_rust fork so that writing "{BUILD_SCM_REVISION?}" deletes the variables if not present. Maybe this could even be accepted upstream?

Alternatively, this could be achieved with some custom bazel rule to produce either an empty file or a version file, depending on whether stamping is enabled or not, and then give the target to rustc_env_file, but this could be more trouble than it is worth. Something like:

version_file_or(
    name = "ott_version_file",
    default_file = "//path/to/empty/file",
)
genrule(
  name = "ott_env_file",
  src = [":ott_version_file"],
  outs = ["stamp-env.txt"],
  cmd = "<inset awk script>", # convert from "KEY VALUE" to "KEY=VALUE"
)

[a-will]

The Starlark and BUILD files seem good to me. I didn't get a chance to look at the opentitantool bits.

[a-will]

Not for this PR, but at some point, we may want to also accept user-provided versioning info. If others are to use a release from this repo for tools and IP, the opentitan git revision is not necessarily the source for it (if a release is even encapsulated in a git repo at all!).

[pamaury]

Yes that's a good point. To some extent this is already possible by customizing the util/get_workspace_status.sh script but if necessary, we could introduce a bazel mecanism to override this.

[cfrantz]

Perhaps this is quibbling on terms, but the user-provided versioning info is encoded in BUILD_GIT_VERSION. That value is the output of git describe and communicates the last git tag, the "distance" from the tag (ie: number of commits) and a git object identifier of the current head at build time.

In the case where one is building exactly at the tag, the extra info (distance, object id) are omitted and the value is exactly the name of the tag.

IMHO, the tag name is the user-provided versioning info as users are responsible for naming the tags. Is there some additional perspective about what we should consider as user-provided versioning info?

[a-will]

Ahh, I'm talking about when the bazel workspace is no longer in a git repo. Once you make a release tarball, there's no more git describe. :)

[pamaury]

I think this is a fair point, we do have any mecanism that works outside the git repository. It seems to me that the purpose of a release could actually include the production of a file that captures the output of the workspace status during the release, and that is then used by bazel instead of running the script. I am not entirely certain this could be achieved without the passing a config file to bazel or releasing a modified BUILD/WORKSPACE file.

[github-actions]

Successfully created backport PR for earlgrey_es_sival:

• Cherry-pick to earlgrey_es_sival: Rework how bazel stamping work in the repository #21168

[jwnrt]

I'm seeing these warnings in the Bazel output with this change applied, but they don't look fatal. Were these always here or from this PR?

I don't think they warrant reverting, but we should address them soon.

INFO: From Action hw/ip/edn/data/edn_regs.h:
opentitan/util/reggen/gen_cheader.py:215: UserWarning: Cannot generate a module define of type logic [31:0]
  warnings.warn("Cannot generate a module define of type {}"
INFO: From Action hw/top_earlgrey/alert_handler_regs.h:
opentitan/util/reggen/gen_cheader.py:215: UserWarning: Cannot generate a module define of type logic [NAlerts-1:0][NLpgWidth-1:0]
  warnings.warn("Cannot generate a module define of type {}"
opentitan/util/reggen/gen_cheader.py:215: UserWarning: Cannot generate a module define of type logic [NAlerts-1:0]
  warnings.warn("Cannot generate a module define of type {}"
INFO: From Action hw/top_earlgrey/alert_handler_regs.h:
opentitan/util/reggen/gen_cheader.py:215: UserWarning: Cannot generate a module define of type logic [NAlerts-1:0][NLpgWidth-1:0]
  warnings.warn("Cannot generate a module define of type {}"
opentitan/util/reggen/gen_cheader.py:215: UserWarning: Cannot generate a module define of type logic [NAlerts-1:0]
  warnings.warn("Cannot generate a module define of type {}"
INFO: From Action hw/ip/edn/data/edn_regs.h:
opentitan/util/reggen/gen_cheader.py:215: UserWarning: Cannot generate a module define of type logic [31:0]
  warnings.warn("Cannot generate a module define of type {}"

[pamaury]

I have seen these warnings for ages on my machine.

[nbdd0121]

Yeah, these are unlikely to be relevant.

[aning12345]

Hi all， I have an ERROR while building the latest version:
ERROR: Traceback (most recent call last):
File "/rv/opentitan-20240307/opentitan/rules/autogen.bzl", line 75, column 7, in
} | stamp_attr(-1, "//rules:stamp_flag"),

Error: unsupported binary operation: dict | dict

[pamaury]

Hi all， I have an ERROR while building the latest version: ERROR: Traceback (most recent call last): File "/rv/opentitan-20240307/opentitan/rules/autogen.bzl", line 75, column 7, in } | stamp_attr(-1, "//rules:stamp_flag"),

Error: unsupported binary operation: dict | dict

Hi, just to clarify what do you mean by "the latest version"? Are you using the bazelisk.sh script in the repository? If not, which version of bazel?
Per the bazel documentation: The union expression x | y yields a new dictionary by combining two existing dictionaries. So this is surprising!

[a-will]

Hi all， I have an ERROR while building the latest version: ERROR: Traceback (most recent call last): File "/rv/opentitan-20240307/opentitan/rules/autogen.bzl", line 75, column 7, in } | stamp_attr(-1, "//rules:stamp_flag"),

Error: unsupported binary operation: dict | dict

Sounds like the incorrect bazel version. The union operator ostensibly became available in 6.0.0, which is still older than the required version (6.2.1).

[aning12345]

yes， I update bazel version, it is ok now. BTW, if my network is limited, how to resolve the Error as below:
Error downloading [https://static.rust-lang.org/dist/2023-10-05/rustc-nightly-x86_64-unknown-linux-gnu.tar.gz] to /rv/.cache/bazel/_bazel_ning/1b677cabd696bf267ab38566915a2e95/external/rustfmt_nightly-2023-10-05__x86_64-unknown-linux-gnu_tools/temp16369494883206175619/rustc-nightly-x86_64-unknown-linux-gnu.tar.gz: connect timed out

please comment ,thank you very much