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

Clear runfiles environment variables for bazel run #17690

Closed
wants to merge 1 commit into from
Closed

Clear runfiles environment variables for bazel run #17690

wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Mar 8, 2023

When an environment variable such as RUNFILES_DIR is set in the client environment when a target using runfiles libraries is run via bazel run, the libraries can't look up the runfiles directory or manifest.

This is fixed by clearing the runfiles-related environment variables from the env in which the target is executed.

Fixes #17571

@fmeum fmeum force-pushed the runfiles-env-variables branch from a66283c to 29eca80 Compare March 8, 2023 16:15
@sgowroji sgowroji added the team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. label Mar 8, 2023
@fmeum fmeum force-pushed the runfiles-env-variables branch from 29eca80 to 144403d Compare March 8, 2023 16:36
@fmeum fmeum marked this pull request as ready for review March 8, 2023 17:20
@fmeum
Copy link
Collaborator Author

fmeum commented Mar 8, 2023 •
edited
Loading

@linzhp Could you test that this is working as expected in your use case?

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 8, 2023

@meteorcloudy This is ready for review. I didn't find a way to e2e test the new behavior as the test framework itself relies on runfiles variables to find bazel.

@linzhp
Copy link
Contributor

linzhp commented Mar 9, 2023

It works! Thanks for fixing it

@fmeum fmeum force-pushed the runfiles-env-variables branch 2 times, most recently from da9d5a7 to 06f1788 Compare March 9, 2023 07:09
@fmeum
Copy link
Collaborator Author

fmeum commented Mar 9, 2023

I added a test for the --script_path case as I could figure out how to do that without messing with the test framework setup itself.

@fmeum fmeum force-pushed the runfiles-env-variables branch 2 times, most recently from 5a3130f to cc84d31 Compare March 9, 2023 09:13
@meteorcloudy meteorcloudy added team-Local-Exec Issues and PRs for the Execution (Local) team and removed team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. labels Mar 9, 2023
@meteorcloudy meteorcloudy self-requested a review March 9, 2023 11:05
@meteorcloudy meteorcloudy requested a review from Wyverald March 9, 2023 11:06
@meteorcloudy meteorcloudy added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Mar 9, 2023
@meteorcloudy
Copy link
Member

@fmeum We are having some trouble importing this PR.
Can you move the integration test from //src/test/shell/integration:run_test to //src/test/shell/bazel:run_test? Because the first one also needs to run internally, but external repo like @bazel_tools doesn't exist.

@sgowroji We can re-import this PR when the issue is fixed.

@fmeum
Clear runfiles environment variables for bazel run
ef19a29 
When an environment variable such as `RUNFILES_DIR` is set in the client
environment when a target using runfiles libraries is run via
`bazel run`, the libraries can't look up the runfiles directory or
manifest.

This is fixed by clearing the runfiles-related environment variables
from the env in which the target is executed.
@fmeum fmeum force-pushed the runfiles-env-variables branch from cc84d31 to ef19a29 Compare March 13, 2023 12:13
@fmeum
Copy link
Collaborator Author

fmeum commented Mar 13, 2023

@meteorcloudy I moved the test.

@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Mar 14, 2023
@fmeum fmeum deleted the runfiles-env-variables branch March 15, 2023 16:04
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
@fmeum @fweikert
Clear runfiles environment variables for bazel run
606b17f 
When an environment variable such as `RUNFILES_DIR` is set in the client environment when a target using runfiles libraries is run via `bazel run`, the libraries can't look up the runfiles directory or manifest.

This is fixed by clearing the runfiles-related environment variables from the env in which the target is executed.

Fixes bazelbuild#17571

Closes bazelbuild#17690.

PiperOrigin-RevId: 516474822
Change-Id: Ia5201d4334b286b36ba2e476e850b98992ca0ffa
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 22, 2023

@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 Sep 22, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 22, 2023

This also helps with bazel-in-bazel invocations, which are common with bazel run.

@meteorcloudy
Copy link
Member

@bazel-io fork 6.4.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 Sep 22, 2023
@bazel-io bazel-io mentioned this pull request Sep 22, 2023
Closed
@iancha1992
Copy link
Member

iancha1992 commented Sep 22, 2023 •
edited
Loading

@fmeum @meteorcloudy @Wyverald I tried to cherry-pick this to release-6.4.0, but looks like there are some conflicts.

  1. src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
  • public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult options) from release-6.4.0 and master, have conflicts I cannot resolve.
  • private static ExecRequest buildExecRequest should already be in the release-6.4.0 branch, but it's currently missing

cc: @bazelbuild/triage

fmeum added a commit to fmeum/bazel that referenced this pull request Sep 23, 2023
@fmeum
Clear runfiles environment variables for bazel run
4720e67 
When an environment variable such as `RUNFILES_DIR` is set in the client environment when a target using runfiles libraries is run via `bazel run`, the libraries can't look up the runfiles directory or manifest.

This is fixed by clearing the runfiles-related environment variables from the env in which the target is executed.

Fixes bazelbuild#17571

Closes bazelbuild#17690.

PiperOrigin-RevId: 516474822
Change-Id: Ia5201d4334b286b36ba2e476e850b98992ca0ffa
@fmeum fmeum mentioned this pull request Sep 23, 2023
Merged
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 23, 2023

@iancha1992 I submitted a manual cherry-pick in #19606.

meteorcloudy pushed a commit that referenced this pull request Sep 25, 2023
@fmeum
[6.4.0] Clear runfiles environment variables for bazel run (#19606)
65e5c37 
When an environment variable such as `RUNFILES_DIR` is set in the client
environment when a target using runfiles libraries is run via `bazel
run`, the libraries can't look up the runfiles directory or manifest.

This is fixed by clearing the runfiles-related environment variables
from the env in which the target is executed.

Fixes #17571

Closes #17690.

PiperOrigin-RevId: 516474822
Change-Id: Ia5201d4334b286b36ba2e476e850b98992ca0ffa

Closes #19596
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meteorcloudy meteorcloudy meteorcloudy approved these changes

@Wyverald Wyverald Awaiting requested review from Wyverald

Assignees
No one assigned
Labels
team-Local-Exec Issues and PRs for the Execution (Local) team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bazel run should not inherit RUNFILES_DIR from OS
6 participants
@fmeum @linzhp @meteorcloudy @iancha1992 @bazel-io @sgowroji