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

fix: correctly obtain relative path required for the venv created by --bootstrap_impl=script #2439

Merged
merged 12 commits into from
Nov 25, 2024

Conversation

chowder
Copy link
Contributor

@chowder chowder commented Nov 23, 2024

Computing the relative path from the venv interpreter to the underlying interpreter was
incorrectly using the actual interpreter's directory depth, not the venv interpreter's
directory depth, when computing the distance from the venv interpreter to the runfiles root.
The net effect is the correct relative path would only be computed for binaries with
the same directory depth as the actual interpreter (e.g. 2).

This went undetected in CI because the tests for this logic just happen to have the
same directory depth as the actual interpreter used.

To fix, compute the relative path to the runfiles root using the venv interpreter
directory. Also added a test in a more nested directory to test this case.

Along the way:

  • Change relative path computation to compute a minimum relative path.
  • Fix the internals to pass a runfiles-root relative path, not main-repo relative path,
    for the actual interpreter, as intended.

Fixes #2169

@chowder chowder marked this pull request as draft November 23, 2024 20:12
@ewianda
Copy link
Contributor

ewianda commented Nov 24, 2024

The following rules were tested and validated:

  • py_binary
  • py_test
  • py_console_script
  • [] rules_oci Python image

@chowder chowder changed the title fix: correctly count the escapes required for the venv created by --bootstrap_impl=script fix: correctly obtain relative path required for the venv created by --bootstrap_impl=script Nov 24, 2024
Copy link
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, hah. I'm guessing the reason this worked in CI is the path depth of the test programs was the same as the path depth of the underlying interpreter? 😂

Thanks for the PR!

The relative path logic looks overly complicated, though? All it has to do is calculate how many up-escapes are needed to get from the {prefix}.venv/bin/python3 location to the runfiles root, then append the ../-stripped value of the actual interpreter to that.

I think the ../ prefixed part of e.g. runtime.interpreter.short_path (or any .short_path) is there in lieu of external/, and represents "go up one dir from the main repo" (because its referring to some external repo).

@chowder
Copy link
Contributor Author

chowder commented Nov 24, 2024

Yeah it's kinda complicated (don't like it) - I wasn't sure what were all the edge cases here, so writing it generically was the only way I knew how to do it.

Would something like this suffice?

        venv_bin_dir = paths.dirname(interpreter.short_path)

        if venv_bin_dir.startswith("../"):
            escapes = venv_bin_dir.count("/") - 1
        else:
            escapes = venv_bin_dir.count("/") + 1

        parent = "/".join([".."] * escapes)
        rel_path = parent + "/" + interpreter_actual_path
        ctx.actions.symlink(output = interpreter, target_path = rel_path)

(Wouldn't support a py_binary using a python binary checked into the main repo as a toolchain, unless we prepend the workspace name to interpreter_actual_path where it doesn't lead with ../)

Happy for you to replace this with whatever you see fit too. :)

@rickeylev rickeylev marked this pull request as ready for review November 25, 2024 00:19
@rickeylev
Copy link
Collaborator

would something like this suffice

Yes, it should. I pushed a change that basically did that -- compute the relative path between two runfiles-root-relative paths instead of main-repo-relative paths. I kept the relative_path function since I like that it will compute a minimum relative-path.

I also pushed a test to reproduce the problem. It creates a test in a much more deeply nested directory than where the actual interpreter is. As well as some other cleanup.

@rickeylev
Copy link
Collaborator

I wasn't sure what were all the edge cases here

I think you got them all, so good job 🙂 . That short_path is a main-repo relative path, and gets ../ prepended for external things is the main edge case. The code was a bit confusing because it said it was passing runfiles-root-relative paths around, but it actually wasn't (interpreter_actual_path was the plain short_path value, which then got fixed up later), and the some later code was papering over that. I went and fixed that.

… update real examples to use runfiles-root relative paths
@rickeylev
Copy link
Collaborator

The CI failure is due to bazel-contrib/bazel_features#82, so I'll force-merge past it.

@rickeylev
Copy link
Collaborator

Ah, actually, I don't think I can force-merge because I'm not an admin. I'll ask about getting admin back. Otherwise, we can just switch the "upcoming bazel" CI to 8rc2 instead of 8rc3

@chowder
Copy link
Contributor Author

chowder commented Nov 25, 2024

@rickeylev Thanks for the changes. :)

@rickeylev rickeylev merged commit 438b12e into bazelbuild:main Nov 25, 2024
3 of 4 checks passed
@ewianda
Copy link
Contributor

ewianda commented Dec 1, 2024

Apologies. Just noticed that rules_pkg is failing

  File "/home/ewianda/.cache/bazel/_bazel_ewianda/00aeeab5df0e41ab9e87aa07d33c6a73/sandbox/linux-sandbox/43/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/rules_pkg~/pkg/private/tar/build_tar.runfiles/_main/../rules_pkg~/pkg/private/tar/build_tar.py", line 486, in <module>
    main()
  File "/home/ewianda/.cache/bazel/_bazel_ewianda/00aeeab5df0e41ab9e87aa07d33c6a73/sandbox/linux-sandbox/43/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/rules_pkg~/pkg/private/tar/build_tar.runfiles/_main/../rules_pkg~/pkg/private/tar/build_tar.py", line 477, in main
    output.add_manifest_entry(entry, file_attributes)
  File "/home/ewianda/.cache/bazel/_bazel_ewianda/00aeeab5df0e41ab9e87aa07d33c6a73/sandbox/linux-sandbox/43/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/rules_pkg~/pkg/private/tar/build_tar.runfiles/_main/../rules_pkg~/pkg/private/tar/build_tar.py", line 342, in add_manifest_entry
    self.add_file(entry.src, entry.dest, **attrs)
  File "/home/ewianda/.cache/bazel/_bazel_ewianda/00aeeab5df0e41ab9e87aa07d33c6a73/sandbox/linux-sandbox/43/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/rules_pkg~/pkg/private/tar/build_tar.runfiles/_main/../rules_pkg~/pkg/private/tar/build_tar.py", line 111, in add_file
    self.tarfile.add_file(
  File "/home/ewianda/.cache/bazel/_bazel_ewianda/00aeeab5df0e41ab9e87aa07d33c6a73/sandbox/linux-sandbox/43/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/rules_pkg~/pkg/private/tar/build_tar.runfiles/rules_pkg~/pkg/private/tar/tar_writer.py", line 251, in add_file
    with open(file_content, 'rb') as f:
         ^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: 'bazel-out/k8-fastbuild/bin/external/rules_pkg~/pkg/private/tar/_build_tar.venv/bin/python3'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plain invocation of python subprocess doesn't inherit sys.path for bootstrap_impl=script
4 participants