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

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

Merged
merged 1 commit into from
Jan 25, 2023

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented Jan 4, 2023

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
Copy link
Collaborator

@chrislovecnm chrislovecnm left a comment

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.

.bazelrc Outdated Show resolved Hide resolved
examples/build_file_generation/WORKSPACE Outdated Show resolved Hide resolved
gazelle/.gitignore Outdated Show resolved Hide resolved
gazelle/python/python_test.go Outdated Show resolved Hide resolved
@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
Collaborator

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

Copy link
Collaborator

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

LGTM

@alexeagle
Copy link
Collaborator

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

@chrislovecnm
Copy link
Collaborator

@alexeagle who else can review it?

@alexeagle
Copy link
Collaborator

alexeagle commented Jan 11, 2023

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
Copy link
Collaborator Author

aignas commented Jan 12, 2023

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
Collaborator

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 bazelbuild#972 easier to review. This PR is only moving
files and addressing a few small review comments made in the initial
review of bazelbuild#972.

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

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

Work towards bazelbuild#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 bazelbuild#972 easier to review. This PR is only moving
files and addressing a few small review comments made in the initial
review of bazelbuild#972.

Work towards bazelbuild#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
Copy link
Collaborator Author

aignas commented Jan 24, 2023

@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 bazelbuild:main Jan 25, 2023
@f0rmiga
Copy link
Collaborator

f0rmiga commented Jan 25, 2023

Thanks for addressing this, @aignas!

@aignas aignas deleted the gazelle-bzlmod-5 branch January 25, 2023 22:39
ianpegg-bc pushed a commit to ianpegg-bc/rules_python that referenced this pull request May 12, 2023
feat!(gazelle): Move the plugin to a separate workspace

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

None yet

4 participants