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 imports when --experimental_python_import_all_repositories=false #593

Closed
wants to merge 1 commit into from
Closed

Conversation

aschleck
Copy link

@aschleck aschleck commented Jun 9, 2022

This fixes breakage for users of this repository when --experimental_python_import_all_repositories=false is set, which the Bazel team would like to do in bazelbuild/bazel#2636.

Without this change, users get the error:

  File "/home/april/.cache/bazel/_bazel_april/4d8027e6d32eeb7619bc14d5897e6dd1/sandbox/linux-sandbox/34/execroot/blah/bazel-out/k8-opt-exec-2B5CBBC6/bin/external/rules_pkg/pkg/private/tar/build_tar.runfiles/rules_pkg/pkg/private/tar/build_tar.py", line 22, in <module>
    from pkg.private import archive
ModuleNotFoundError: No module named 'pkg'

Because imports can always be done with the absolute workspace name, changing these imports still works fine for tests in this repository and for users who have --experimental_python_import_all_repositories=true.

I am not clear on the intention of not_named_rules_pkg in the tests and what the implications are of me changing that to rules_pkg. It was important to change because otherwise the source files would all need to import not_named_rules_pkg.blah.blah which of course would be nonsense.

There is some error in the build rules for //tests/rpm:analysis_tests_conflicting_inputs_base that occurs at head, but otherwise all tests appear to pass with this change.

@google-cla
Copy link

google-cla bot commented Jun 9, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

This fixes breakage when --experimental_python_import_all_repositories=false is set, which the Bazel team would like to do in bazelbuild/bazel#2636.
@aiuto
Copy link
Collaborator

aiuto commented Jun 10, 2022

The issue you mention is 5 years old and would break a lot of people, so I don't think that flip is going to happen.
In general, you should not use any --incompatible-* flags unless there is absolutely no way to avoid them. They are transitional, for testing and migration, and may go away at any time.

I think this could break people who vendor the code into their repositories and have their own rewrites for import paths or PYPATH lookup. I know it will break my own workflow. I'll be mostly unavailable for any low priority rules_pkg work until sometime in July. I will look at this then.

@aschleck
Copy link
Author

Thank you for taking a look so quickly!

I acknowledge that the flag may die an untimely death. The scenario I am trying to avoid is where I am pulling Python dependencies from another workspace with generically named root directories (tools/pkg/python) and my own repository also defines a library package at the root level with the same name. Consider that if my code does from bazel_tools.tools.python.runfiles import runfiles I am no longer able to import from a tools package in my own repository (without importing with the workspace prefix, like this CL proposes.) I suppose if the flag goes away my code would break but it's easier to debug that than it is for people to debug "why can't I import from tools" suddenly when some far away dep adds a dependency on the runfiles.

Anyway I understand if the risk of breakage here is too much, but would you mind sharing a little more about your vendoring and rewriting? My understanding of vendoring Bazel workspaces boiled down to liberal use of local_repository but it sounds like you're doing something else. I'm always curious to learn more about how folks leverage Bazel.

@aiuto
Copy link
Collaborator

aiuto commented Jul 15, 2022

To answer the early question, the "not_named_rules_pkg" is an important integration test case that makes sure we can run if the repo is named something else. This will be critical in the bzlmod world, because if people depend on distinct rules_pkg vesrsions they will get different repo names. That alone may make much of this PR impossible, because the imports will break.

For vendoring, what Google (and Bazel) does, is usually not local repositories. We import the unpacked dependency to //third_party/... so that all the source is directly visible. That let's us index and code search it. If there are local mods, we tend to write patch files to re-apply the mod from the sources, so that they are easy to re-apply after an update from upstream. So, what we end up doing is rewriting from pkg.foo import bar as from third_party.rules_pkg.pkg.foo import bar. That is pretty ugly, because you have to know that pkg.foo needs the third_party prefix but something_else.foo might not. We reduce the pain through careful use of copybara scripts.

@aschleck
Copy link
Author

Thank you for the detailed explanation, will close this.

@aschleck aschleck closed this Jul 18, 2022
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