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.

Sign up for GitHub

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

Jump to bottom

Make env on test rules override --test_env #22420

Closed
wants to merge 1 commit into from
Closed

Make env on test rules override --test_env #22420

wants to merge 1 commit into from

Conversation

keith
Copy link
Member

@keith keith commented May 16, 2024

Fixes #14418

@keith keith force-pushed the ks/make-env-on-test-rules-override-test_env branch from 0ddb8e6 to 2b4265c Compare May 16, 2024 21:03
@keith keith mentioned this pull request May 16, 2024
Closed
@keith keith marked this pull request as ready for review May 16, 2024 21:34
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label May 16, 2024
@iancha1992 iancha1992 added the team-Rules-Server Issues for serverside rules included with Bazel label May 16, 2024
@keith keith force-pushed the ks/make-env-on-test-rules-override-test_env branch from 2b4265c to 6c23b63 Compare May 16, 2024 22:55
@comius
Copy link
Contributor

comius commented May 17, 2024

hey @lberki, do you perhaps know if this ordering/overwriting was intentional or accidental?

With this PR, there might be a risk that a Starlark rule overwrites PATH variable or some other important variables. (or it might be a feature). Or I might be overthinking it.

@lberki
Copy link
Contributor

lberki commented May 17, 2024

Doing some code archaeology (I went back as far as cl/146237448), it looks like this ordering was what Blaze did before it was Bazel and no one wanted to change the semantics for fear of breaking some legacy behavior.

@comius comius added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels May 17, 2024
@comius
Copy link
Contributor

comius commented May 17, 2024

Doing some code archaeology (I went back as far as cl/146237448), it looks like this ordering was what Blaze did before it was Bazel and no one wanted to change the semantics for fear of breaking some legacy behavior.

Ok. So let's see if that's still there.

@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label May 21, 2024
@keith keith deleted the ks/make-env-on-test-rules-override-test_env branch May 21, 2024 16:25
@keith
Copy link
Member Author

keith commented May 21, 2024

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 21, 2024
@iancha1992
Copy link
Member

@bazel-io fork 7.2.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 21, 2024
@bazel-io bazel-io mentioned this pull request May 21, 2024
Closed
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request May 21, 2024
@keith @bazel-io
Make env on test rules override --test_env
b5d3fc6 
Fixes bazelbuild#14418

Closes bazelbuild#22420.

PiperOrigin-RevId: 635740423
Change-Id: Iffd4d172c4175be2e1b6cfad04ddad2759adb987
@bazel-io bazel-io mentioned this pull request May 21, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request May 21, 2024
@bazel-io @keith
[7.2.0] Make env on test rules override --test_env (#22473)
87b0a1f 
Fixes #14418

Closes #22420.

PiperOrigin-RevId: 635740423
Change-Id: Iffd4d172c4175be2e1b6cfad04ddad2759adb987

Commit
01a90d6

Co-authored-by: Keith Smiley <keithbsmiley@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmeum fmeum fmeum left review comments

@comius comius comius approved these changes

Assignees
No one assigned
Labels
team-Rules-Server Issues for serverside rules included with Bazel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

env attribute of *_test rules should be able to overwrite variables like PATH
6 participants
@keith @comius @lberki @iancha1992 @fmeum @bazel-io