Skip to content

feat(gazelle): For package mode, resolve dependencies when imports are relative to the package path - #2865

Merged
aignas merged 6 commits into
bazel-contrib:mainfrom
yushan26:resolve-rel-imports
Jun 22, 2025
Merged

feat(gazelle): For package mode, resolve dependencies when imports are relative to the package path#2865
aignas merged 6 commits into
bazel-contrib:mainfrom
yushan26:resolve-rel-imports

Conversation

@yushan26

@yushan26 yushan26 commented May 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

When # gazelle:python_generation_mode package is enabled, relative imports are currently not being added to the deps field of the generated target.

For example, given the following Python code:

from .library import add as _add
from .library import divide as _divide
from .library import multiply as _multiply
from .library import subtract as _subtract

The expected py_library rule should include a dependency on the local library package:

py_library(
    name = "py_default_library",
    srcs = ["__init__.py"],
    visibility = ["//visibility:public"],
    deps = [
        "//example/library:py_default_library",
    ],
)

However, the actual generated rule is missing the deps entry:

py_library(
    name = "py_default_library",
    srcs = ["__init__.py"],
    visibility = ["//visibility:public"],
)

This change updates file_parser.go to ensure that relative imports (those starting with a .) are parsed and preserved. In Resolve(), logic is added to correctly interpret relative paths:

A single dot (.) refers to the current package.

Multiple dots (.., ..., etc.) traverse up parent directories.

The relative import is resolved against the current label.Pkg path that imports the module and converted into an path relative to the root before dependency resolution.

As a result, dependencies for relative imports are now correctly added to the deps field in package generation mode.

Added a directive # gazelle:experimental_allow_relative_imports true to allow this feature to be opt in.

@yushan26
yushan26 requested review from aignas and dougthor42 as code owners May 7, 2025 22:42
@yushan26
yushan26 force-pushed the resolve-rel-imports branch from 37c9386 to 8914db1 Compare May 7, 2025 23:08
linzhp
linzhp reviewed May 8, 2025
View reviewed changes
Comment thread gazelle/python/testdata/relative_imports/BUILD.in
linzhp
linzhp reviewed May 9, 2025
View reviewed changes
Comment thread gazelle/python/testdata/resolve_deps_relative_imports/BUILD.in Outdated
linzhp
linzhp approved these changes May 21, 2025
View reviewed changes

@linzhp linzhp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. @aignas Can you take a look?

@aignas

aignas commented May 21, 2025

Copy link
Copy Markdown
Collaborator

Don't have time right now to look into gazelle PRs at a great detail, so I'll defer this one to @dougthor42, Thanks @linzhp, for the initial review!

@dougthor42

dougthor42 commented May 23, 2025

Copy link
Copy Markdown
Collaborator

@yushan26 can you take a look at the discussion in #2203? There are some edge case issues that I'd like to know if you've addressed. It looks like you're focusing on the "importing a class/function identifier" case. Does this also support importing relative modules?

from ..my_library import some_function  # looks like you've got this covered
from ...my_library.foo import some_function  # this too
from .library import other_module
from .. import some_module
from .. import some_function  # eg the function is exposed in ../__init__.py

Also, if we add support for relative imports, we need to make sure that it works when # gazelle:python_generation_mode file.

@linzhp

linzhp commented May 23, 2025

Copy link
Copy Markdown
Contributor

Also, if we add support for relative imports

To clarify, relative imports are already supported before this PR (see this test), but it's partially broken in package mode (on importing another package) and not supported in file mode. This PR fixes the package mode. I am leaning towards fixing the file mode in a separate PR, unless you feel strongly about fixing both of them in one PR.

@yushan26
yushan26 force-pushed the resolve-rel-imports branch from 24ace24 to 7d1f434 Compare May 23, 2025 22:55
@yushan26

yushan26 commented May 23, 2025

Copy link
Copy Markdown
Contributor Author

Yea good point. Updated the test case to support the scenario and updated the function.

dougthor42
dougthor42 reviewed May 24, 2025
View reviewed changes

@dougthor42 dougthor42 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am leaning towards fixing the file mode in a separate PR

SGTM.

Other requests:

  1. Please update the PR title to follow the guidelines.
  2. Please update the changelog.

Comment thread gazelle/python/resolve.go Outdated
Comment thread gazelle/python/resolve.go Outdated
Comment thread gazelle/python/testdata/relative_imports_package_mode/README.md Outdated
Comment thread ...lle/python/testdata/relative_imports_package_mode/package1/subpackage1/subpackage2/script.py Outdated
Comment thread ...lle/python/testdata/relative_imports_package_mode/package1/subpackage1/subpackage2/script.py Outdated
Comment thread gazelle/python/testdata/relative_imports_project_mode/README.md Outdated
@yushan26 yushan26 changed the title Resolve dependencies when imports are relative to the package path feat(gazelle) For package mode, resolve dependencies when imports are relative to the package path May 27, 2025
@yushan26
yushan26 requested a review from rickeylev as a code owner May 27, 2025 20:19
@yushan26

yushan26 commented May 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@dougthor42 Thanks for the review. I also included the directive # gazelle:experimental_allow_relative_imports to allow opt-in instead.

@dougthor42 dougthor42 changed the title feat(gazelle) For package mode, resolve dependencies when imports are relative to the package path feat(gazelle): For package mode, resolve dependencies when imports are relative to the package path May 30, 2025
dougthor42
dougthor42 reviewed May 30, 2025
View reviewed changes

@dougthor42 dougthor42 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

With the new config option, please update gazelle/README.md with the directive details: an entry in the directives table and a detailed section (example).

Comment thread CHANGELOG.md Outdated
Comment thread CHANGELOG.md Outdated
@yushan26
yushan26 force-pushed the resolve-rel-imports branch from a0e53f3 to 847fdae Compare June 3, 2025 18:31
@linzhp

linzhp commented Jun 16, 2025

Copy link
Copy Markdown
Contributor

@aignas can you merge this one?

dougthor42
dougthor42 reviewed Jun 21, 2025
View reviewed changes
Comment thread gazelle/README.md Outdated
@dougthor42
dougthor42 enabled auto-merge June 21, 2025 23:55
@dougthor42
dougthor42 added this pull request to the merge queue Jun 21, 2025
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 21, 2025
@aignas
aignas added this pull request to the merge queue Jun 22, 2025
Merged via the queue into bazel-contrib:main with commit 036e8c5 Jun 22, 2025
@jimmyt857 jimmyt857 mentioned this pull request Oct 25, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dougthor42 dougthor42 dougthor42 approved these changes
@aignas aignas Awaiting requested review from aignas aignas is a code owner
@rickeylev rickeylev Awaiting requested review from rickeylev rickeylev is a code owner
+1 more reviewer
@linzhp linzhp linzhp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yushan26 @aignas @dougthor42 @linzhp @yushan8