rollup_bundle should have a stamp attr to control if that target should ever use stamping

[joeljeske]

🚀 feature request

Relevant Rules

rollup_bundle(
  stamp = 0 # or 1
)

Description

rollup_bundle (and I presume other packaging steps like pkg_npm) include the version file as a dep whenever --stamp is passed. I believe that bazel typically pairs this stamping config with a stamp attr on each rule that might use stamping info. This allows targets to declare that they never use stamping information, even if --stamp is passed (for other targets).

Describe the solution you'd like

If you have a solution in mind, please describe it.

rollup_bundle(
  stamp = 0 # or 1
)

If stamp is 0, never include the version file. If stamp is 1, include the version file if --stamp is passed.

Describe alternatives you've considered

Have you considered any alternative solutions or workarounds?

This should be supported inside rollup_bundle, not attempted to be patched from the outside.

[alexeagle]

I'm surprised that you'd want a build with --stamp in which some targets don't get stamped. What's the use case for that?
Also you say " I believe that bazel typically pairs this stamping config" - could you point to any examples of that?

[joeljeske]

I'm referring to the behavior that I understand from java_binary where stamp = 1|0|-1

Integer; optional; default is -1

Enable link stamping. Whether to encode build information into the binary. Possible values:

stamp = 1: Stamp the build information into the binary. Stamped binaries are only rebuilt when their dependencies change. Use this if there are tests that depend on the build information.
stamp = 0: Always replace build information by constant values. This gives good build result caching.
stamp = -1: Embedding of build information is controlled by the --[no]stamp flag.

My use case is that I have lots of rollup_bundles for each of my intermediate packages. These packages are linked together at the end in a web archive using importmaps and systemjs without re-processing or re-bundling. Most of these rollup_bundles don't leverage stamping since they are not the final product, however, I have some downstream tasks (like pkg_npm) that I want stamping enabled. So I guess the issue is that the rollup_bundle is presuming that the presence of --stamp is always relevant itself and thus it uses the volatile version file as a dep even though I will never use it in some rules. This makes it always misses the remote cache (bazelbuild/bazel#10075) which makes it less performant than it could be

[joeljeske]

Any more thoughts on this?

[alexeagle]

Oops, finally cleaning up stamping now. #1441 gave us the same behavior as the java_binary default of -1 - if you pass --stamp to the build, we sense that (via a config_setting like rules_go) and add the stamp vars for rollup to read. This is like rules_go. IMO it's not a good idea that every stamping-capable rule has to repeat a stamp attribute, do you know why that's useful on java_binary?

[joeljeske]

It's valuable for rules to be able to always opt out of stamping for a given target (0 behavior). It's also valuable to have the 1 behavior, since then one could control stamping or not based on anything configurable and not just the stamp flag.

[alexeagle]

It makes the docs more complex for users, so I'd still like more data on why those are valuable features vs. only having --stamp to control it.

[joeljeske]

Often, we may want to stamp some stamp some things in a build, but not other things. The --stamp is a global flag that makes all things be stamped. Just because some rule supports stamping, does not mean I want to stamp every usage of the rule and invalidate downstream caching.

[alexeagle]

Oh, actually there's already a way to do this, it's just less obvious than what you propose.

https://github.com/bazelbuild/rules_nodejs/blob/ba4fdb83a05b1e7cb0af7f92dbd2229f5eda2586/internal/pkg_npm/test/BUILD.bazel#L49 shows how I stamp an action even without --stamp - you just need to pass a NodeContextInfo to any stamp-aware rule

[alexeagle]

WDYT about resolving this issue by updating the docs for node_context_data attributes, instead of just "Internal use only" it could say

Provides info about the build context, such as stamping. By default it reads from the bazel command line, such as the --stamp argument. Use this to override values for this target, such as enabling or disabling stamping.

but not document this in the stamping.md user doc because I think it's probably just confusing there

[joeljeske]

Yea that could work fine then, I would just like to be able to control the stamping beyond the global flag. Sounds good!