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

Python wrapper sometimes puts manifest filename into RUNFILES_DIR #17675

Closed
mafanasyev-tri opened this issue Mar 7, 2023 · 2 comments
Closed
Labels

Comments

@mafanasyev-tri
Copy link

mafanasyev-tri commented Mar 7, 2023

Description of the bug:

py_binary rules come with a auto-generated wrapper script which automatically detects runfiles and sets either RUNFILES_DIR or RUNFILES_MANIFEST_FILE, depending on what is found first.

This script has a bug: if progname.runfiles_manifest file is missing, but programe.runfiles/MANIFEST is present, the script erroneously sets RUNFILES_DIR to manifest file location. This causes all sorts of failures down the line (for example when using bazel's runfiles.bash rules)

Code in question:

runfiles = os.path.join(module_space, 'MANIFEST')

(compare to previous stanza)

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

To demonstrate it, we need any bazel repo which uses python runfiles lib. Bazel's own rules_pkg is one example.
First, check out and run the test. It should work:

git clone https://github.com/bazelbuild/rules_pkg.git
cd rules_pkg
bazel run //tests:stamp_test  # This works

Now naively package the runfiles dir and try to use it. Note we've got stamp_test.runfiles/MANIFEST (inside runfiles) but forgot stamp_test.runfiles_manifest:

rm -rf /tmp/work && mkdir /tmp/work && cp -r bazel-bin/tests/stamp_test bazel-bin/tests/stamp_test.runfiles /tmp/work && /tmp/work/stamp_test
...
NotADirectoryError: [Errno 20] Not a directory: '/tmp/work/stamp_test.runfiles/MANIFEST/rules_pkg/tests/stamped_zip.zip'

----------------------------------------------------------------------
Ran 2 tests in 0.002s

FAILED (errors=2)

observe how it treats stamp_test.runfiles/MANIFEST file as a directory and fails.

Which operating system are you running Bazel on?

Various Ubuntu versions (18.04, 20.04, 22.04)

What is the output of bazel info release?

This is present in HEAD and was introduced in 2018. Testcases above were tested on 6.0.0

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 ?

https://github.com/bazelbuild/bazel.git
4427a1fb0cf537da82e1ca4841e0cecda99e1107
4427a1fb0cf537da82e1ca4841e0cecda99e1107

Have you found anything relevant by searching the web?

Nothing. I am guessing very few people use custom bazel packaging scripts that only copy one file but not both.

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

We have a workaround which is to copy both files, and it probably is more correct that way, too. I am reporting this bug for the unlikely case someone else encounters it.

Workarounds for my test case above:

  • Include both manifest files:
rm -rf /tmp/work && mkdir /tmp/work && cp -r bazel-bin/tests/stamp_test{,.runfiles,.runfiles_manifest} /tmp/work && /tmp/work/stamp_test
..
OK
  • Do not include any manifest files
rm -rf /tmp/work && mkdir /tmp/work && cp -r bazel-bin/tests/stamp_test{,.runfiles} /tmp/work && rm -f /tmp/work/stamp_test.runfiles/MANIFEST && /tmp/work/stamp_test
..
OK
@mafanasyev-tri
Copy link
Author

Updated ticket description to include test case.

keertk added a commit that referenced this issue Apr 18, 2023
… using a manifest (#18133)

Unfortunately, we weren't able to find a way to reproduce the reported bug
in a test environment, but the line of code in question is obviously wrong,
so we'll just omit a test to cover this.

Fixes #17675

Closes #17722.

PiperOrigin-RevId: 525044539
Change-Id: I7e1eaa14eac1d4dabcdcf93d92720c41977b1fe2

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 2023
… using a manifest

Unfortunately, we weren't able to find a way to reproduce the reported bug
in a test environment, but the line of code in question is obviously wrong,
so we'll just omit a test to cover this.

Fixes bazelbuild#17675

Closes bazelbuild#17722.

PiperOrigin-RevId: 525044539
Change-Id: I7e1eaa14eac1d4dabcdcf93d92720c41977b1fe2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants
@mafanasyev-tri and others