search

bazelbuild / bazel-skylib

Common useful functions and rules for Bazel
https://bazel.build/
Apache License 2.0
391 stars 178 forks source link

Add env attr to native_binary and native_test #409

Open cameron-martin opened 1 year ago

cameron-martin commented 1 year ago

native_binary and native_test currently have no way to specify the environment that the target runs with. A RunEnvironmentInfo provider could be returned from these, based on a passed-in env attr.

cameron-martin commented 1 year ago

This env param does work even without RunEnvironmentInfo (similar to args).

fmeum commented 1 year ago

@cameron-martin Are you sure that's true? As far as I know the args parameter is treated specially for all rules, but env is only for native rules.

cameron-martin commented 1 year ago

Looks like I'm too eager to clean up my issues list. I'll reopen this as you're probably right about this.

redsun82 commented 9 months ago

Hi! I would also like this! For now I'm working around this with a patch, using this rather small change:

https://github.com/bazelbuild/bazel-skylib/compare/main...redsun82:bazel-skylib:env-native-binary?expand=1

I would have contributed it as a PR, but I'm a bit unsure about:

fmeum commented 9 months ago

@redsun82 You could use bazel_features, which provides you a way to test for certain global symbols: https://github.com/bazel-contrib/bazel_features/tree/main?tab=readme-ov-file#accessing-globals RunEnvironmentInfo is already supported: https://github.com/bazel-contrib/bazel_features/blob/443861571a389ddc16d17690ab8e46ee87b4ea57/private/globals.bzl#L5

redsun82 commented 9 months ago

ah, nice one @fmeum

redsun82 commented 9 months ago

For the moment I've opened this draft PR, I'll update it sometime soon with the RunEnvironmentInfo check and have a look at the existing native_binary tests.

redsun82 commented 9 months ago

I've updated the PR, which turned out a bit trickier than I thought. For the moment I decided to replicate part of what bazel_features does to avoid complicating the legacy workflow-based integration instructions, which is not ideal. If you deem that we should rather go for bazel_features to simplify the code but update the instructions to integrate bazel_skylib into a WORKFLOW file accordingly with the new dependency, I can update the PR further.

redsun82 commented 8 months ago

I've opened another PR based on the previous one but using bazel_features (and modifying the release instructions for WORKSPACE setup). There was a bit of gymnastics involved in getting the doc update right, but I guess the two approaches can be compared now.

redsun82 commented 3 months ago

I've got a first approval on https://github.com/bazelbuild/bazel-skylib/pull/484 a couple of weeks ago (thanks @jgertm!). I've now updated the PR, and added location expansion to env variables (I actually have a use case for passing LD_PRELOAD with a library in data).