Skip to content

Conversation

@jsing-canva
Copy link

@jsing-canva jsing-canva commented Jul 14, 2025

The current _FindPythonRunfilesRoot() implementation assumes that
the Python module has been unpacked four levels below the runfiles
directory. This is not the case in multiple situations, for example when
rules_pycross is in use and has installed the module via pypi (in which
case it is five levels below runfiles).

Both strategies already know where the runfiles directory exists -
implement _GetRunfilesDir() on the _DirectoryBased strategy, then call
_GetRunfilesDir() in order to populate self._python_runfiles_dir.

Stop passing a bogus path to runfiles.Create() in testCurrentRepository(),
such that the test actually uses the appropriate runfiles path.

Fixes #3085

@aignas
Copy link
Collaborator

aignas commented Oct 11, 2025

Hello, going through old PRs and was wondering if you would like to rebase this and see if you can get the CI pass? Recently there were some changes in runfiles to make the compact manifest work, so this may require some work to cleanly rebase.

The current _FindPythonRunfilesRoot() implementation assumes that
the Python module has been unpacked four levels below the runfiles
directory. This is not the case in multiple situations, for example when
rules_pycross is in use and has installed the module via pypi (in which
case it is five levels below runfiles).

Both strategies already know where the runfiles directory exists -
implement _GetRunfilesDir() on the _DirectoryBased strategy, then call
_GetRunfilesDir() in order to populate self._python_runfiles_dir.

Stop passing a bogus path to runfiles.Create() in testCurrentRepository(),
such that the test actually uses the appropriate runfiles path.

Fixes bazel-contrib#3085
@jsing-canva jsing-canva force-pushed the runfiles-fix-upstream branch 2 times, most recently from 45b6f72 to da225b5 Compare October 23, 2025 00:59
@jsing-canva
Copy link
Author

Hello, going through old PRs and was wondering if you would like to rebase this and see if you can get the CI pass? Recently there were some changes in runfiles to make the compact manifest work, so this may require some work to cleanly rebase.

Thanks for the follow up - have rebased.

I had a closer look at the CI failures (Windows) yesterday and will need some guidance on how you want to proceed. The issue is caused by what appears to be a bug in the zip file bootstrap, which is being used by Windows in the two failing tests (//tests/runtime_env_toolchain:bootstrap_script_test, //tests/runtime_env_toolchain:toolchain_runs_test). The issue is caused by RUNFILES_DIR being incorrectly set to the location where the first stage/zip is unpacked, rather than the actual module space (the temporary Bazel.runfiles_*). This results in the following failure:

ValueError: C:\temp\Bazel.runfiles_9i4gshx2\runfiles\rules_python\tests\runtime_env_toolchain\toolchain_runs_test.py does not lie under the runfiles root c:\b\uzvqz6q5\execroot\rules_python\bazel-out\x64_windows-fastbuild-st-ac131bc5af1e\bin\tests\runtime_env_toolchain\bootstrap_script_test.exe.runfiles

Running the same code/test via a zip from a py_binary works correctly (and the other runfiles tests pass). I've not tracked down the exact source of the zip file bootstrap, however it is not part of the rules_python repo and is presumably coming from the toolchain that is being used in the test (commenting out the extra_toolchains part of the tests results in the runfiles code working, then failing due to other reasons, which seems to confirm this).

So given all of this, I'm not sure how we want to proceed... we either need a toolchain that does not have a buggy zip file bootstrap, or we need to skip this particular test (at least the runfiles part of it) on Windows.

Preferences/suggestions?

@jsing-canva
Copy link
Author

(I've pushed two changes - one that adds an actual Rlocation() data file test to //tests/runfiles and another that adds a workaround for the broken zip bootstrap in //tests/runtime_env_toolchain - hopefully this is acceptable)

@jsing-canva
Copy link
Author

@aignas any chance we can move this forward?

@aignas
Copy link
Collaborator

aignas commented Nov 5, 2025

We'll need to fix our CI before we can merge.

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.

bazel-runfiles: incorrect assumption for Python runfiles root

2 participants