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

bootstrap_impl=script doesn't handle RUNFILES_MANIFEST_FILE #2186

Closed
rickeylev opened this issue Sep 5, 2024 · 2 comments · Fixed by #2177
Closed

bootstrap_impl=script doesn't handle RUNFILES_MANIFEST_FILE #2186

rickeylev opened this issue Sep 5, 2024 · 2 comments · Fixed by #2177

Comments

@rickeylev
Copy link
Contributor

🐞 bug report

Affected Rule

py_binary with --boostrap_impl=script

Is this a regression?

Yes, bootstrap=system_python doesn't have this problem

Originally reported by @scasagrande in #2177

Description

When bootstrap=script is used, it doesn't properly handle when RUNFILES_MANIFEST_FILE is used to find the runfiles directory.

Looking at the code, both code paths that look for *.runfiles/MANIFEST and *.runfiles_manifest might have the same problem.

The fix is to reappend .runfiles to the file name after stripping off the manifest location.

🔬 Minimal Reproduction

To repro, these flags must be set: --enable_runfiles=false --build_runfiles_manifests=true --spawn_strategy=local

It seems that the RUNFILES_MANIFEST_FILE envvar is only set for local executions when runfiles are disabled.

@scasagrande
Copy link
Contributor

small correction: flag is --build_runfile_manifests (runfile, not runfiles)

@rickeylev
Copy link
Contributor Author

The difficulty in reproducing this might related to a bazel bug: bazelbuild/bazel#7994

Basically, RUNFILES_MANIFEST_FILE isn't set when it should be under sandboxed execution.

Testing this will be annoying. We can't modify --enable_runfiles using a transition (it doesn't error, just gets ignored), so have to set it on the command line. Otherwise, tags=local can force local running and a transition can affect --build_runfile_manifests

I'm thinking it'll just be easier to have the shell test modify the env vars and call the binary again.

github-merge-queue bot pushed a commit that referenced this issue Sep 5, 2024
…ES_MANIFEST_FILE set (#2177)

The script-based bootstrap wasn't computing the correct runfiles
directory when
`RUNFILES_MANIFEST_FILE` was set. The path it computed stripped off the
manifest
file name, but didn't re-add the `.runfiles` suffix to point to the
runfiles
directory.

To fix, just re-append the `.runfiles` suffix after it removes the
manifest file
name portion of the path.

Reproducing this is a bit tricky and it's difficult to reproduce the
necessary build
flags in a test; all of the following must be met:
* `--enable_runfiles=false`, but this cannot be set by transitions, only
via command line
* `--build_runfile_manifests=true` (this can be set in a transition, but
see below)
* Due to bazelbuild/bazel#7994, even if a
manifest is created,
the RUNFILES_MANIFEST_FILE env var won't be set _unless_ the test
strategy is local
  (i.e. not sandboxed, which is the default).

To work around those issues, the test just recreates the necessary
envvar state and
invokes the binary. The underlying files may not exist, but that's OK
for the code
paths were testing.

Fixes #2186

---------

Co-authored-by: Richard Levasseur <rlevasseur@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants