rules_js icon indicating copy to clipboard operation
rules_js copied to clipboard

feat: add BAZEL env variable to js_binary

Open gregmagolan opened this issue 7 months ago • 7 comments

Differentiating between Bazel and non-Bazel environments is very common and to date users have been using either BAZEL_TEST or JS_BINARY__* env vars. The former is not set by bazel under bazel run and the latter is not particularly ergonomic. BAZEL is simpler and easier to remember.


Type of change

  • New feature or functionality (change which adds functionality)

Test plan

  • New test cases added

gregmagolan avatar Nov 10 '23 14:11 gregmagolan

Did you search for a conventional variable set in other languages? What do ppl do there?

If you take the CI vendors & tools as an example, they all set CI and each vendor/tool also sets their own specific env var such as BUILDKITE, CIRCLECI, GITLAB_CI, GITHUB_ACTIONS, TRAVIS, APPVEYOR.

https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables

GITHUB_ACTIONS | Always set to true when GitHub Actions is running the workflow. You can use this variable to differentiate when tests are being run locally or by GitHub Actions.

https://buildkite.com/docs/pipelines/environment-variables#buildkite-environment-variables

BUILDKITE #This value cannot be modified | Always true

https://circleci.com/docs/variables/#built-in-environment-variables

CIRCLECI | GitHub OAuth, GitHub App, Bitbucket, GitLab | Boolean | true (represents whether the current environment is a CircleCI environment)

Setting BAZEL seems analogous to these so that programs can detect their environment from a simple well-known env var. I think Bazel should handle this in the core since bazel run a tool is a common enough use can in all ecosystems.

gregmagolan avatar Nov 10 '23 15:11 gregmagolan

I haven't seen the bazel run case handled in other ecosystems. Cases I've seen, users are keying off of BAZEL_TEST for test targets or setting explicit envs such as BAZEL on the target to detect if under Bazel for bazel run.

gregmagolan avatar Nov 10 '23 16:11 gregmagolan

Bazel itself sets BAZEL_TEST though, not rules_js? Should we avoid setting anything without a JS pre/suffix? How about RULES_JS or JS_TEST|RUN|BUILD etc?

jbedard avatar Nov 10 '23 20:11 jbedard

Bazel itself sets BAZEL_TEST though, not rules_js? Should we avoid setting anything without a JS pre/suffix? How about RULES_JS or JS_TEST|RUN|BUILD etc?

Yes, tho BAZEL_TEST is only set when you run bazel test so there is a gap for users that run programs with bazel run to be able to detect in an easy and ergonomic way they are underly Bazel. An env var such as JS_RUN doesn't read well in end-user code since it isn't obvious that you're checking for bazel that way.

There is precedent for setting BAZEL_* var in rules_js already in js_run_binary:

    fixed_env = {
        "BAZEL_BINDIR": "$(BINDIR)",
        "BAZEL_BUILD_FILE_PATH": "$(BUILD_FILE_PATH)",
        "BAZEL_COMPILATION_MODE": "$(COMPILATION_MODE)",
        "BAZEL_PACKAGE": native.package_name(),
        "BAZEL_TARGET_CPU": "$(TARGET_CPU)",
        "BAZEL_TARGET_NAME": name,
        "BAZEL_TARGET": "$(TARGET)",
        "BAZEL_WORKSPACE": "$(WORKSPACE)",
    }

gregmagolan avatar Nov 10 '23 22:11 gregmagolan

I'm just nitpicking this because every added variable has the potential to collide with something the user already set, and shorter is more likely.

alexeagle avatar Nov 11 '23 15:11 alexeagle

I'm just nitpicking this because every added variable has the potential to collide with something the user already set, and shorter is more likely.

I do check if it is already set and don't set override it if it is. Perhaps I should create an issue on bazelbuild/bazel so that bazel itself sets BAZEL to 1 in all cases including bazel run?

gregmagolan avatar Nov 11 '23 16:11 gregmagolan

A bit related: https://github.com/bazelbuild/bazel/issues/15470

alexeagle avatar Dec 01 '23 19:12 alexeagle