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

Correct handling of imports rule attribute on py... rules #6599

Closed
andponlin-canva opened this issue Jul 29, 2024 · 6 comments
Closed

Correct handling of imports rule attribute on py... rules #6599

andponlin-canva opened this issue Jul 29, 2024 · 6 comments
Assignees
Labels
awaiting-maintainer Awaiting review from Bazel team on issues product: Android Studio Android Studio plugin product: CLion CLion plugin product: GoLand GoLand plugin product: IntelliJ IntelliJ plugin product: PyCharm PyCharm plugin type: bug

Comments

@andponlin-canva
Copy link
Contributor

andponlin-canva commented Jul 29, 2024

Description of the bug:

Note: See the associated PR.

I have marked all IDEs because I assume they will all experience the same problem when handling Python targets.

Some of the rules_python rules such as py_library and py_binary have the ability to supply an imports attribute. For example consider the following Bazel repo setup;

//
├── WORKSPACE
└── aaa
    └── bbb
        └── ccc
            ├── BUILD.bazel
            ├── ddd
            │   └── eee
            │       ├── doo.py
            │       └── fff
            │           └── poe.py
            └── zzz
                └── main.py

The BUILD.bazel may carry the following rule;

py_library(
    name = "lib",
    srcs = [
        "ddd/eee/doo.py",
        "ddd/eee/fff/poe.py",
    ],
...
)

At the point of use in main.py, some Python library functions in those files would be;

from aaa.bbb.ccc.ddd.eee.doo import print_fruit
from aaa.bbb.ccc.ddd.eee.fff.poe import print_tree

If the rule is changed to introduce the imports attribute...

py_library(
  name = "lib",
...
  imports = ["ddd"]
...
)

...then the associated Python module path is rooted at the same point as the BUILD.bazel file and includes the imports. At the point of use in main.py, the reference to the library functions would become;

from eee.doo import print_fruit
from eee.fff.poe import print_tree

The expected behavior of the Intelli-J plugin would be to identify the use of imports attribute in py_... rules and to adjust the module paths accordingly. The actual behavior seems to be to ignore the imports attribute.

Changes in can be made to support the imports feature. The data from the imports attribute needs to be fed through from the executed aspect via the proto and can then be post-processed in AbstractPyImportResolverStrategy to replicate the observed behavior of Bazel.

Which category does this issue belong to?

Intellij, GoLand, CLion, PyCharm, Android Studio

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Please find attached a tar-ball support-py-imports-example-20240729.tgz.

  1. Unpack the tar-ball
  2. Ensure that it can be run with bazelisk run "//aaa/bbb/ccc:prog". On output you should see;
    Feijoa
    Rimu
    
  3. Now import the project into Intelli-J.
  4. Sync the project
  5. Open the BUILD.bazel file
  6. Observe the imports is set to ddd
  7. Open the file main.py
  8. Observe that the imports are highlighted as errors in the IDE
  9. Adjust the imports' paths to be;
    from aaa.bbb.ccc.ddd.eee.doo import print_fruit
    from aaa.bbb.ccc.ddd.eee.fff.poe import print_tree
    
  10. Observe that the imports are no longer highlighted as errors in the IDE

The Intelli-J plugin should be accepting the paths in consideration of the imports.

Which Intellij IDE are you using? Please provide the specific version.

Intelli-J 2024.1

What programming languages and tools are you using? Please provide specific versions.

Python 3.10

What Bazel plugin version are you using?

7.2

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

Screenshots

Screenshot before without the patch in place;

support-py-imports-example-before

Screenshot after with the patch in place;

support-py-imports-example-after

Further known issue

There is a further complication that occurs when intermediate module parts are empty. For example;

//
└── a
    └── b
        └── c
            ├── BUILD.bazel
            └── d
                └── e
                    └── f
                        └── doo.py

If the imports here were ["d"] then the IDE plugin would fail to recognize the module part e because it does not contain anything. This issue does not cover this scenario; it is considered to be a separate issue.

@andponlin-canva andponlin-canva added awaiting-maintainer Awaiting review from Bazel team on issues type: bug labels Jul 29, 2024
@github-actions github-actions bot added product: Android Studio Android Studio plugin product: CLion CLion plugin product: GoLand GoLand plugin product: IntelliJ IntelliJ plugin product: PyCharm PyCharm plugin labels Jul 29, 2024
@andponlin-canva
Copy link
Contributor Author

PR merged - thanks. Closing this issue.

@aarontsharp
Copy link

@andponlin-canva I think this may also resolve #350 🎊

@andponlin-canva
Copy link
Contributor Author

Hello @aarontsharp ; From a quick read of that issue, I suspect it may be due to the problem I outlined above under "Further known issue" rather than the plugin not observing the imports rule attribute?

@aarontsharp
Copy link

aarontsharp commented Aug 20, 2024

Hmm, I pulled the example included in #350 here and it seems to be working when using the plugin including this fix (modules are correctly resolved and you can navigate to them). My understanding of that issue is it's caused by the plugin only considering imports from the root of the workspace rather than the imports statement, which I believe this fix addresses.

The further issue you bring up may also be in scope, but at least the example shared in the issue seems to be resolved by this fix

@andponlin-canva
Copy link
Contributor Author

Thanks; OK it is good to hear it has resolved that problem too!

@tpasternak
Copy link
Collaborator

This contribution is great, thank you @andponlin-canva !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-maintainer Awaiting review from Bazel team on issues product: Android Studio Android Studio plugin product: CLion CLion plugin product: GoLand GoLand plugin product: IntelliJ IntelliJ plugin product: PyCharm PyCharm plugin type: bug
Projects
None yet
Development

No branches or pull requests

5 participants