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

bazel run should not inherit RUNFILES_DIR from OS #17571

Closed
linzhp opened this issue Feb 23, 2023 · 6 comments
Closed

bazel run should not inherit RUNFILES_DIR from OS #17571

linzhp opened this issue Feb 23, 2023 · 6 comments
Labels
help wanted Someone outside the Bazel team could own this P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug

Comments

@linzhp
Copy link
Contributor

linzhp commented Feb 23, 2023

Description of the bug:

Runfile libraries such as github.com/bazelbuild/rules_go/go/runfiles relies on the environment variables RUNFILES_DIR and RUNFILES_MANIFEST_FILE to work correctly. These two variables are set by Bazel during bazel run. However, if they are set in OS, Bazel would inherit them and not set them anymore, leading to machine dependent behaviors.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

  1. Create a repo like this:
-- WORKSPACE --
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

http_archive(
    name = "io_bazel_rules_go",
    sha256 = "56d8c5a5c91e1af73eca71a6fab2ced959b67c86d12ba37feedb0a2dfea441a6",
    urls = [
        "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.37.0/rules_go-v0.37.0.zip",
        "https://github.com/bazelbuild/rules_go/releases/download/v0.37.0/rules_go-v0.37.0.zip",
    ],
)

load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")

go_rules_dependencies()

go_register_toolchains(version = "1.19.3")
-- BUILD.bazel --
load("@io_bazel_rules_go//go:def.bzl", "go_binary")

go_binary(
    name = "runfiles",
    srcs = ["main.go"],
    data = ["@go_sdk//:tools"],
    deps = ["@io_bazel_rules_go//go/runfiles"],
)
-- main.go --
package main

import (
        "fmt"
        "log"

        "github.com/bazelbuild/rules_go/go/runfiles"
)

func main() {
        loc, err := runfiles.Rlocation("go_sdk/bin/gofmt")
        if err != nil {
                log.Fatal(err)
        }
        fmt.Println(loc)
}
  1. Run:
RUNFILES_DIR=/tmp bazel run //:runfiles

It outputs /tmp/go_sdk/bin/gofmt, but Bazel should ignore RUNFILES_DIR=/tmp, and output something like /private/var/tmp/_bazel_zplin/a20570cf42c983bd18597126a6b3c06e/external/go_sdk/bin/gofmt instead.

Which operating system are you running Bazel on?

macOS

What is the output of bazel info release?

release 6.0.0-homebrew

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@fmeum
Copy link
Collaborator

fmeum commented Feb 24, 2023

The list of variables potentially affecting runfiles libraries further includes JAVA_RUNFILES, PYTHON_RUNFILES and RUNFILES_MANIFEST_ONLY.

@meteorcloudy @Wyverald What do you think, could bazel run drop these variables from the environment? Not doing so potentially breaks the runfiles library "promise" to "just work" with bazel run.

@sgowroji sgowroji added type: bug team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. untriaged labels Feb 24, 2023
@meteorcloudy
Copy link
Member

In general, I think it makes sense, but I wonder under what circumstances, users would set those env vars before running bazel run? And what if the user just runs the binary directly, it could still be a problem, right?

@meteorcloudy meteorcloudy added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels Feb 24, 2023
@fmeum
Copy link
Collaborator

fmeum commented Feb 24, 2023

I'll defer to @linzhp for the first question.

Running a binary directly already has some notable differences from running it via bazel run, e.g. args an env aren't honored. I don't think users would expect everything to just work if they do that, but they should probably be able to assume that bazel run makes it so.

@linzhp
Copy link
Contributor Author

linzhp commented Feb 24, 2023

users would set those env vars before running bazel run?

This is just a way to reproduce the issue. In the real scenario, they set RUNFILES_DIR in their $HOME/.zshrc file when they were debugging something else in the past and forgot to remove it. When we enable Python Gazelle extension, which uses the runfiles library to locate Python parser, they found Gazelle no longer work for them.

And what if the user just runs the binary directly, it could still be a problem, right?
In such scenario, it's users responsibility to set RUNFILES_DIR correctly, or pass the files via command line arguments.

@meteorcloudy
Copy link
Member

I see, thanks for the context! I'm happy to review a PR to address this issue ;)

@meteorcloudy meteorcloudy added the help wanted Someone outside the Bazel team could own this label Feb 27, 2023
fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 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 bazelbuild#17571

Closes bazelbuild#17690.

PiperOrigin-RevId: 516474822
Change-Id: Ia5201d4334b286b36ba2e476e850b98992ca0ffa
@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 fmeum 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
fmeum added a commit to fmeum/bazel that referenced this issue Sep 23, 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 bazelbuild#17571

Closes bazelbuild#17690.

PiperOrigin-RevId: 516474822
Change-Id: Ia5201d4334b286b36ba2e476e850b98992ca0ffa
meteorcloudy pushed a commit that referenced this issue Sep 25, 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

Closes #17690.

PiperOrigin-RevId: 516474822
Change-Id: Ia5201d4334b286b36ba2e476e850b98992ca0ffa

Closes #19596
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 6.4.0 RC2. Please test out the release candidate and report any issues as soon as possible. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Someone outside the Bazel team could own this P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants