Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add stamping support to go_binary #3980

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ian-h-chamberlain
Copy link
Contributor

@ian-h-chamberlain ian-h-chamberlain commented Jul 3, 2024

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Adds a stamp attribute to go_binary which can be used to override --[no]stamp behavior for a specific binary target.

Semantics match those of cc_binary, and are limited to just go_binary for now.

Which issues(s) does this PR fix?

Closes #2154

Other notes for review

Open questions / tasks:

  • I am not 100% sure if there's additional documentation that needs to be updated, if someone can point me in the right direction I'm happy to add that.
  • Should go_test have this attribute as well? I didn't think it made sense for libraries have it, since conflicts between dependencies would need to be resolved, but I could probably add it for _test without needing to address those.

Copy link
Member

@fmeum fmeum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ternary value for stamp is generally pretty confusing, even if consistent with cc_binary. Could you explain what you would like to use this new attribute for? I wonder whether we really need to support all three values.

@ian-h-chamberlain
Copy link
Contributor Author

Sure, the use case we have in my organization is for iterating while manually testing one of our apps. We basically have two main use cases:

  • Building for production via CI (we use --stamp in the build)
  • Developer build for local testing (requires remembering --stamp every time to get a stamped build)

We don't want to stamp everything (non-Go, unit tests, etc.) all of the time, to maximize cache hits, so we don't have --stamp in our .bazelrc. However, we would ideally like to use stamping for developer builds to embed e.g. git data, build timestamp just for the final go_binary.

As it stands without this change (or equivalent), users have to remember to pass --stamp every time they build this particular target to get the embedded metadata, but with this change we could set stamp = 1 and forget it. For our purposes we would also be fine with stamp = True to force-stamp a particular target (where the default behavior for False would be to just follow --[no]stamp).

I mainly kept the ternary in case there would ever be users that want to forcefully disable stamping for certain targets, but I'm not sure if is particularly common. A quick search on GitHub shows some usage like this, but it seems fairly limited (mostly forks of llvm-bazel, I guess): https://github.com/search?q=%2F%5Cbstamp+%3D+0%2F+path%3A%2FBUILD%28%5C.bazel%29%3F%24%2F&type=code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add stamp attribute in go_binary rule
2 participants