Skip to content
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

feat: add BAZEL env variable to js_binary #1351

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gregmagolan
Copy link
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
Member

@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
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
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
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
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
Member

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
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
Member

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
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
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants