-
Notifications
You must be signed in to change notification settings - Fork 432
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
rustc_env_file
can now render workspace status stamp format strings
#983
Conversation
d624bfd
to
a63ecaf
Compare
@hlopko Friendly ping, would love to get these changes in 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone with more C++ than me should probably review the C++, but otherwise looks great to me! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor things, but looks good to me. I apologize for the delay.
) | ||
|
||
def stamp_build_setting(name, visibility = ["//visibility:public"]): | ||
native.config_setting( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any issue with this implementation, but here https://docs.bazel.build/versions/main/skylark/lib/transition.html#transition I see that //command_line_option:cpu
works for transitions for accessing --cpu
, so maybe //command_line_option:stamp
works as well and can be used as any other build settings. May be worth trying out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation was inspired by rules_pkg which has been in place for some time. I'd rather stick with the same approach since it seems to have withstood the test of time (so far).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. But by trying to use //command_line_option:stamp
you may discover that it's already possible to access value of --stamp
flag from Starlark today, negating the need for the workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(But if I had to bet, I'd bet on //command_line_option
thing not working as a normal build setting, I'd expect the label only exists in the context of configuration transitions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two options may not be what you were thinking but demonstrating that they don't work.
diff --git a/rust/private/stamp.bzl b/rust/private/stamp.bzl
index 1a7cab6..3292225 100644
--- a/rust/private/stamp.bzl
+++ b/rust/private/stamp.bzl
@@ -48,7 +48,7 @@ def stamp_build_setting(name, visibility = ["//visibility:public"]):
_stamp_build_setting(
name = name,
value = select({
- ":stamp_detect": True,
+ "//command_line_option:stamp": True,
"//conditions:default": False,
}),
visibility = visibility,
ERROR: Analysis of target '//rust/private:stamp' failed; build aborted: no such package 'command_line_option': BUILD file not found in any of the following directories. Add a BUILD file to a directory to mark it as a package.
diff --git a/rust/private/stamp.bzl b/rust/private/stamp.bzl
index 1a7cab6..7f0f963 100644
--- a/rust/private/stamp.bzl
+++ b/rust/private/stamp.bzl
@@ -41,7 +41,7 @@ _stamp_build_setting = rule(
def stamp_build_setting(name, visibility = ["//visibility:public"]):
native.config_setting(
name = "stamp_detect",
- values = {"stamp": "1"},
+ values = {"//command_line_option:stamp": "1"},
visibility = visibility,
)
ERROR: /rules_rust/rust/private/BUILD.bazel:15:20: in values attribute of config_setting rule //rust/private:stamp_detect: error while parsing configuration settings: unknown option: '//command_line_option:stamp'. Since this rule was created by the macro 'stamp_build_setting', the error might have been caused by the macro implementation
I've used //command_line_option
with transitions (exactly what you linked to earlier) but I feel the build setting is simpler than transitions and would opt to keep this as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something I'm happy to make another PR for if I can understand the appropriate change. Since this is an exciting change for some, I'm gonna merge this since ultimately this is an implementation detail that does not impact the API.
@hlopko Thanks for the review! Some quick follow up questions if you don't mind 😅 |
This change allows for
rust_*
targets to specify thestamp
attribute to determine if or how format strings ({VAR}
) will be rendered in files passed to therustc_env_files
attribute.