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

Able to import undeclared dependencies #1354

Closed
matts1 opened this issue Aug 2, 2023 · 1 comment
Closed

Able to import undeclared dependencies #1354

matts1 opened this issue Aug 2, 2023 · 1 comment

Comments

@matts1
Copy link
Contributor

matts1 commented Aug 2, 2023

🐞 bug report

Affected Rule

py_binary/library/test

Is this a regression?

No

Description

Python uses sys.path in a similar manner to how bash uses the path variable. There are two relevant entries here.

sys.path[0] contains the directory of the script being run. This allows you to write imports relative to the binary being run.

sys.path[1] is set to RUNFILES_DIR. This allows you to write imports relative to the workspace root. For example, you would import @foo//bar:baz as from foo.bar import baz.

The unfortunate side effect of the former is that because the path of the script being run is relative to the workspace, not the output directory, you are able to import any python file, not just ones you declared as dependencies.

It is my opinion that the former is an antipattern, and the latter is the only correct way to perform imports (for first party libraries), as if you have the binaries //foo:bin and //bar:bin both importing a library //baz:lib, if baz then writes import blah, then foo attempts to import //foo:blah, while bar attempts to import //bar:blah. However, it must be recognised that this is a common use case, and will not be feasible.

I suggest that we instead provide a flag allowing you to disable the first entry in sys.path to prevent python from importing outside your runfiles. It would:

  • If enabled, remove sys.path[0]
  • If disabled, still remove sys.path[0], but replace it with RUNFILES_DIR / sys.path[0].relative_to(workspace_root). This should still allow you to perform relative imports, but remove the ability to import files you haven't declared a dependency on.

I can implement this myself if required, but I don't know where the correct place to modify the sys.path would be.

🔬 Minimal Reproduction

https://github.com/matts1/rules_python/blob/implicit_deps_repro/examples/bzlmod/demo/test.py

🔥 Exception or Error

None

🌍 Your Environment

Operating System:

  
Debian 6.3.7
  

Output of bazel version:

  
6.0.0
  

Rules_python version:

  
Tested at head (e355becc30275939d87116a4ec83dad4bb50d9e1), but appears to have been present forever.
  

Anything else relevant?

@matts1
Copy link
Contributor Author

matts1 commented Sep 11, 2023

Apparently this is a duplicate of github.com/bazelbuild/bazel/issues/7091

@matts1 matts1 closed this as not planned Won't fix, can't repro, duplicate, stale Sep 11, 2023
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

No branches or pull requests

1 participant