Add env and env_inherit to native_binary and native_test (using @bazel_features)#484
Add env and env_inherit to native_binary and native_test (using @bazel_features)#484redsun82 wants to merge 27 commits intobazelbuild:mainfrom
env and env_inherit to native_binary and native_test (using @bazel_features)#484Conversation
redsun82
requested review from
brandjon,
c-mita,
comius and
tetromino
as code owners
redsun82
changed the title
env and env_inherit to native_binary and native_testenv and env_inherit to native_binary and native_test (using @bazel_features)
Conflicts: MODULE.bazel docs/native_binary_doc.md rules/native_binary.bzl
FYI, going by https://bazel.build/release#support-matrix , v5.4.1 is now the minimum supported version, so if (not a full review, I'll defer to @tetromino / @brandjon for that) |
According to https://bazel.build/release#support-matrix, we can drop support for Bazel 4.x (so we can use `runfiles.merge_all`) and Bazel <5.3 (so we can use `RunEnvironmentInfo` unconditionally, which simplifies the change significantly).
|
@hvadehra thanks for the heads up, that makes things way easier. @tetromino / @brandjon, the PR should be ready for another review pass now 🚀 |
| ) | ||
|
|
||
| bazel_dep(name = "platforms", version = "0.0.4") | ||
| bazel_dep(name = "platforms", version = "0.0.9") |
There was a problem hiding this comment.
there was a warning about a dependency pulling this version in
tests/native_binary/assertenv.cc
Outdated
| #define OS_VAR "HOME" | ||
| #endif | ||
|
|
||
| enum vars_to_be_found { |
There was a problem hiding this comment.
Could you implement this test in a less hacky way?
For example, get rid of the enum, and just search for 3 strings. Use set found to true/false.
comius
left a comment
comius
left a comment
There was a problem hiding this comment.
LGTM (except for the test)
|
@comius sorry for the late update on this, I've cleaned up the test now. |
|
@comius any chance you could have another look at this? |
5 participants
Closes #409