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: dependency resolver on windows only works when --enable_runfiles and --windows_enable_symlinks is used #2457

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ChewyGumball
Copy link

compile_pip_requirements doesn't work as expected on Windows unless both --enable_runfiles and --windows_enable_symlinks are used. Both options default to off on Windows because the filesystem on Windows makes setting up the runfiles directories with actual files very slow. This means that anyone on Windows with a default set up has to search around the Github issues to try and figure out why things don't work as advertised.

The dependency_resolver.py doesn't inherently require these options, it just had two bugs that prevented it from working.

  1. calling pip_compile exits the whole program so it never gets to run the code that should copy the output to the source tree. Things just happen to work on linux because the runfiles are symlinks, and it does not need to copy anything.
  2. it assumed the runfiles resolved file would be in the runfiles tree, but on Windows, when --enable_runfiles is not set, it actually gets resolved to a file in the source tree.

Before:

bazel run //third_party/python:requirements.update
Starting local Bazel server and connecting to it...
INFO: Invocation ID: 8aa3e832-78ce-4999-912b-c43e7ca3212b
INFO: Analyzed target //third_party/python:requirements.update (129 packages loaded, 9563 targets configured).
INFO: Found 1 target...
Target //third_party/python:requirements.update up-to-date:
  bazel-bin/third_party/python/requirements.update.zip
  bazel-bin/third_party/python/requirements.update.exe
INFO: Elapsed time: 60.964s, Critical Path: 0.77s
INFO: 8 processes: 2 remote cache hit, 6 internal.
INFO: Build completed successfully, 8 total actions
INFO: Running command line: bazel-bin/third_party/python/requirements.update.exe '--src=_main/third_party/python/requirements.txt' _main/third_party/python/requirements_lock.txt //third_party/python:requirements.update '--resolver=backtracking' --allow-unsafe --generate-hashes '--requirements-windows=_main/third_party/python/requirements_windows.txt' --strip-extras
Updating third_party/python/requirements_windows.txt
Error: Could not open file 'third_party/python/requirements_windows.txt': No such file or directory

After:

bazel run //third_party/python:requirements.update
INFO: Invocation ID: 39f999a0-6c1d-4b2c-a1be-3d71e838916a
INFO: Analyzed target //third_party/python:requirements.update (5 packages loaded, 45 targets configured).
INFO: Found 1 target...
Target //third_party/python:requirements.update up-to-date:
  bazel-bin/third_party/python/requirements.update.zip
  bazel-bin/third_party/python/requirements.update.exe
INFO: Elapsed time: 5.410s, Critical Path: 4.79s
INFO: 2 processes: 1 internal, 1 local.
INFO: Build completed successfully, 2 total actions
INFO: Running command line: bazel-bin/third_party/python/requirements.update.exe '--src=_main/third_party/python/requirements.txt' _main/third_party/python/requirements_lock.txt //third_party/python:requirements.update '--resolver=backtracking' --allow-unsafe --generate-hashes '--requirements-windows=_main/third_party/python/requirements_windows.txt' --strip-extras
Updating third_party/python/requirements_windows.txt
#
# This file is autogenerated by pip-compile with Python 3.13
# by the following command:
#
#    bazel run //third_party/python:requirements.update
#
mpmath==1.3.0 \
    --hash=sha256:7a28eb2a9774d00c7bc92411c19a89209d5da7c4c9a9e227be8330a23a25b91f \
    --hash=sha256:a0b2b9fe80bbcd81a6647ff13108738cfb482d481d826cc0e02f5b35e5c88d2c
    # via sympy
sympy==1.13.3 \
    --hash=sha256:54612cf55a62755ee71824ce692986f23c88ffa77207b30c1368eda4a7060f73 \
    --hash=sha256:b27fd2c6530e0ab39e275fc9b683895367e51d5da91baa8d3d64db2565fec4d9
    # via -r G:/projects/bedrock-engine/third_party/python/requirements.txt

And //third_part/python:requirements_windows.txt is updated.

Reference issues: #1943 #1431

@aignas
Copy link
Collaborator

aignas commented Nov 29, 2024

Thanks for the PR.

  • Could you please add documentation as part of this PR to the https://github.com/bazelbuild/rules_python/blob/main/docs/pypi-dependencies.md file to explicitly state what the minimum requirements for Windows are? Do we still require these options?
  • Maybe it would be a good idea to change the .bazelrc in the examples to drop the options to ensure that we are testing the Windows part in the CI? What do you think?

…ndalone_mode = False' is not passed. On linux, this ended up not mattering because the file written to by pip_compile is a symlink to the file in the source tree. Symlinks are disabled by default on windows, so it writes to a new file in the runfiles tree but doesn't copy back to the source tree because the program exits.
…noenable_runfiles). This means the runfiles library resolves the requirements_file to the file in the source tree (ie resolved_requirements_file is the absolute path to the file in the source tree, not to a file in the runfiles directory). pip_compile is told to output to a relative path, which means it will output to a file in the runfiles directory. The check to see if we need to copy that file to the source tree was incorrect because it didn't check to see if the file output by pip_compile was the same as the one in the source tree, it checked if the resolved file was the same as the one in the source tree. Since the resolved file IS the same as the one in the source tree, it never copied, but the resolved file is not the file that pip_compile output to.
@ChewyGumball ChewyGumball force-pushed the fix_dependency_resolver_on_windows branch from e85a02d to 3b41efe Compare November 29, 2024 01:40
@ChewyGumball
Copy link
Author

* Could you please add documentation as part of this PR to the https://github.com/bazelbuild/rules_python/blob/main/docs/pypi-dependencies.md file to explicitly state what the minimum requirements for Windows are? Do we still require these options?

There are no longer any minimum requirements on Windows after this PR. I don't see any such existing requirements in that document, so I don't think we need to add anything do we?

* Maybe it would be a good idea to change the `.bazelrc` in the examples to drop the options to ensure that we are testing the Windows part in the CI? What do you think?

The problem is that the test created by compile_pip_requirements doesn't update the file in the source tree, so the existing integration tests can't cover this issue. I don't know how to make a test that would cover this issue because we would need to run the compile_pip_requirements.update target, and I don't think you can run a target from a test.

Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

I'll play with the CI a little later, for now I wanted to leave the comment on the documentation.

CHANGELOG.md Outdated Show resolved Hide resolved
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.

2 participants