Skip to content

feat(gazelle)!: Move the plugin to a separate workspace - #972

Merged
f0rmiga merged 1 commit into
bazel-contrib:mainfrom
aignas:gazelle-bzlmod-5
Jan 25, 2023
Merged

feat(gazelle)!: Move the plugin to a separate workspace#972
f0rmiga merged 1 commit into
bazel-contrib:mainfrom
aignas:gazelle-bzlmod-5

Conversation

@aignas

@aignas aignas commented Jan 4, 2023

Copy link
Copy Markdown
Collaborator

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Previously the gazelle plugin was part of the same WORKSPACE. This means that we cannot have a separate bzlmod module and make the rules_python repo work with both, bzlmod and legacy dependency management systems.

Issue Number: #965

What is the new behavior?

The gazelle plugin is now isolated with its own WORKSPACE file. Whilst making this I have also moved the plugin source code to a separate directory.

Summary:

  • Move go.mod to gazelle.
  • Move gazelle definition.
  • Move the gazelle plugin to a separate folder, just like bazel-skylib does, which helps with naming of the externally visible targets. This is now refactor(gazelle): Move plugin to a separate directory. #983.
  • Fix file distribution for the gazelle module.
  • Update the example test.
  • Include rules_python_gazelle_plugin during integration tests
  • Update ignored packages

Does this PR introduce a breaking change?

  • Yes
  • No

Steps that need to be taken:

  • Add a new dependency to your WORKSPACE and change the import path for the plugin dependency setup.
  • If you are using the gazelle binary from the plugin repo, use the new label @rules_python_gazelle_plugin//python:gazelle_binary.
  • If you are using the gazelle language extension, use the new label @rules_python_gazelle_plugin//python.
  • gazelle_python.yaml integrity gets changed for all users.

@aignas aignas mentioned this pull request Jan 4, 2023
12 tasks
@aignas
aignas force-pushed the gazelle-bzlmod-5 branch 4 times, most recently from 5ae27f4 to abde25c Compare January 4, 2023 10:51
@aignas
aignas marked this pull request as ready for review January 4, 2023 11:01

@chrislovecnm chrislovecnm 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.

Look great. I have some nitpicks that I noticed, but I like it!! Working on the build_file_generation example I was wondering if bzmod and gazelle where working.

Comment thread .bazelrc Outdated
Comment thread examples/build_file_generation/WORKSPACE Outdated
Comment thread gazelle/.gitignore Outdated
Comment thread gazelle/python/python_test.go Outdated
@aignas
aignas force-pushed the gazelle-bzlmod-5 branch 2 times, most recently from 7b0ec08 to 2ad688f Compare January 5, 2023 08:03
@chrislovecnm

Copy link
Copy Markdown
Contributor

@aignas LGTM. Since this is a breaking change, do we need release notes? @f0rmiga can you PTLA?

@chrislovecnm chrislovecnm 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

@alexeagle

Copy link
Copy Markdown
Contributor

FYI @f0rmiga is on paternity leave, back next week I believe

@chrislovecnm

Copy link
Copy Markdown
Contributor

@alexeagle who else can review it?

@alexeagle

alexeagle commented Jan 11, 2023

Copy link
Copy Markdown
Contributor

I think we should probably just wait for @f0rmiga to be back. This will need someone to invest a few hours.

As one way to make this easier to land, @aignas is it possible to split out a separate PR that does the directory rename so this doesn't show as a 433 files changed? Or leave the directory rename for a subsequent refactoring? I can't even get started on reviewing it at this size without setting aside a chunk of my day.

(Also, thank you for taking this on!!)

@aignas

aignas commented Jan 12, 2023

Copy link
Copy Markdown
Collaborator Author

Hey @alexeagle, I also think that waiting for @f0rmiga is probably for the best. I can split it to several commits at the very least. I think the rename is necessary to make everything work as we are moving the gazelle rule usage to the gazelle folder and it makes things a little bit tricky.

What I can do to make it easier to review is to split it into multiple PRs introducing a breaking change:

  • Do the folder rename.
  • Add a new WORKSPACE.
  • Whatever is left afterwards.

How does it sound to you? And sorry for making the initial PR too long, I was mainly looking at the changed lines counter.

@alexeagle

Copy link
Copy Markdown
Contributor

Yeah I think the folder rename first will make it more manageable. Thanks!

aignas added a commit to aignas/rules_python that referenced this pull request Jan 12, 2023
This is in order to make bazel-contrib#972 easier to review. This PR is only moving
files and addressing a few small review comments made in the initial
review of bazel-contrib#972.

Work towards bazel-contrib#965.
aignas added a commit to aignas/rules_python that referenced this pull request Jan 12, 2023
This is in order to make bazel-contrib#972 easier to review. This PR is only moving
files and addressing a few small review comments made in the initial
review of bazel-contrib#972.

Work towards bazel-contrib#965.
aignas added a commit to aignas/rules_python that referenced this pull request Jan 12, 2023
This is in order to make bazel-contrib#972 easier to review. This PR is only moving
files and addressing a few small review comments made in the initial
review of bazel-contrib#972.

Work towards bazel-contrib#965.
@aignas aignas changed the title breaking: feat(gazelle): Move the plugin to a separate workspace feat(gazelle)!: Move the plugin to a separate workspace Jan 12, 2023
aignas added a commit to aignas/rules_python that referenced this pull request Jan 24, 2023
This is in order to make bazel-contrib#972 easier to review. This PR is only moving
files and addressing a few small review comments made in the initial
review of bazel-contrib#972.

Work towards bazel-contrib#965.
@aignas
aignas force-pushed the gazelle-bzlmod-5 branch 2 times, most recently from d1de560 to d9bd232 Compare January 24, 2023 06:31
@aignas

aignas commented Jan 24, 2023

Copy link
Copy Markdown
Collaborator Author

@f0rmiga, I think once you merge #983, this PR should be much more approachable.

Summary:
* Move go.mod to gazelle.
* Move gazelle definition.
* Fix file distribution for the gazelle module.
* Update the example test.
* Include rules_python_gazelle_plugin during integration tests
* Update ignored packages
* Update CI configuration
@f0rmiga
f0rmiga merged commit fd5f531 into bazel-contrib:main Jan 25, 2023
@f0rmiga

f0rmiga commented Jan 25, 2023

Copy link
Copy Markdown
Member

Thanks for addressing this, @aignas!

@aignas
aignas deleted the gazelle-bzlmod-5 branch January 25, 2023 22:39
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.

4 participants