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

fix stamp = 0 in rust_binary_without_process_wrapper #1218

Merged
merged 3 commits into from
Mar 24, 2022

Conversation

krasimirgg
Copy link
Collaborator

Stamping is not supported when building without process_wrapper:

fail("build_env_files, build_flags_files, stamp are not supported when building without process_wrapper")

@krasimirgg krasimirgg marked this pull request as ready for review March 24, 2022 07:23
@krasimirgg krasimirgg requested a review from hlopko March 24, 2022 07:23
@UebelAndre
Copy link
Collaborator

Why not remove the stamping functionality entirely from the rule? There's already some unique logic for generating the attributes:
https://github.com/bazelbuild/rules_rust/blob/0.1.0/rust/private/rust.bzl#L931-L956

@krasimirgg krasimirgg changed the title don't ever stamp when building process_wrapper fix stamp = 0 in rust_binary_without_process_wrapper Mar 24, 2022
@krasimirgg
Copy link
Collaborator Author

Why not remove the stamping functionality entirely from the rule? There's already some unique logic for generating the attributes: https://github.com/bazelbuild/rules_rust/blob/0.1.0/rust/private/rust.bzl#L931-L956

Thank you! Done.

@krasimirgg krasimirgg requested a review from UebelAndre March 24, 2022 07:59
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@UebelAndre UebelAndre merged commit 99b4b25 into bazelbuild:main Mar 24, 2022
@hlopko hlopko requested a review from scentini March 24, 2022 08:12
@hlopko
Copy link
Member

hlopko commented Mar 24, 2022

There is https://github.com/hlopko/rules_rust/blob/19aa1216358e84a0bb538008d333e6356697c49f/rust/private/rustc.bzl#L944 that covers some attributes, and this PR handles stamp differently. We should aim for consistency.

@UebelAndre I actually wanted @scentini to take a look and give feedback before merging. I assumed that by adding them to reviewers this was implied. Would you prefer if I commented something in the spirit "reviewers, please don't merge until we all approve" in the future?

@hlopko
Copy link
Member

hlopko commented Mar 24, 2022

Oh I misread the code, this is actually handling the issue quite nicely, thank you!

@UebelAndre
Copy link
Collaborator

@UebelAndre I actually wanted @scentini to take a look and give feedback before merging. I assumed that by adding them to reviewers this was implied. Would you prefer if I commented something in the spirit "reviewers, please don't merge until we all approve" in the future?

Sorry, next time I'll wait. Wasn't sure what the procedure was. Now I know 😅

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.

3 participants