Skip to content

feat: add BAZEL env variable to js_binary#1351

Closed
gregmagolan wants to merge 1 commit into2.xfrom
add_BAZEL_to_env
Closed

feat: add BAZEL env variable to js_binary#1351
gregmagolan wants to merge 1 commit into2.xfrom
add_BAZEL_to_env

Conversation

@gregmagolan
Copy link
Copy Markdown
Member

@gregmagolan gregmagolan commented Nov 10, 2023

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 gregmagolan marked this pull request as ready for review November 10, 2023 14:45
Copy link
Copy Markdown
Contributor

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

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

@gregmagolan gregmagolan force-pushed the add_BAZEL_to_env branch 2 times, most recently from 696fe59 to d636729 Compare November 10, 2023 14:51
@gregmagolan
Copy link
Copy Markdown
Member Author

gregmagolan commented Nov 10, 2023

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 gregmagolan force-pushed the add_BAZEL_to_env branch 3 times, most recently from db6c713 to c4f5663 Compare November 10, 2023 16:42
@gregmagolan
Copy link
Copy Markdown
Member Author

gregmagolan commented Nov 10, 2023

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.

@jbedard
Copy link
Copy Markdown
Member

jbedard commented Nov 10, 2023

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?

@gregmagolan
Copy link
Copy Markdown
Member Author

gregmagolan commented Nov 10, 2023

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)",
    }

@alexeagle
Copy link
Copy Markdown
Contributor

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.

@gregmagolan
Copy link
Copy Markdown
Member Author

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 gregmagolan self-assigned this Nov 27, 2023
@gregmagolan gregmagolan added the enhancement New feature or request label Nov 27, 2023
@alexeagle
Copy link
Copy Markdown
Contributor

A bit related: bazelbuild/bazel#15470

@alexeagle alexeagle removed their request for review December 15, 2023 17:26
@gregmagolan gregmagolan mentioned this pull request May 6, 2024
21 tasks
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Greg Magolan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jbedard jbedard closed this Feb 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants