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

Move gazelle plugin into a new git repo #423

Closed
tetromino opened this issue Jan 17, 2023 · 8 comments
Closed

Move gazelle plugin into a new git repo #423

tetromino opened this issue Jan 17, 2023 · 8 comments

Comments

@tetromino
Copy link
Collaborator

After #400, I don't understand why the gazelle plugin belongs in this git repo:

  • skylib and the gazelle plugin now have separate bazel workspaces
  • gazelle plugin doesn't get built by bazel build //...
  • gazelle plugin doesn't get tested by bazel test //...
  • gazelle plugin depends only on skylib's public api
  • skylib does not depend on the gazelle plugin at all

Originally posted by @tetromino in #410 (comment)

@tetromino
Copy link
Collaborator Author

Potentially interested parties: @Wyverald @brandjon @shs96c @achew22 @comius

@achew22
Copy link
Member

achew22 commented Jan 17, 2023

While only speaking for myself here, I think that having the plugin live in the same place as the rules is ultimately the way to do things for most, if not all, cases.

If everything is in one place, rules authors are able to simultaneously make changes to their gazelle plugin and their rules set in the same commit. Git tags are applied at consistent points so there is no risk of drift. This is how it's done for rules_python and they seem very happy with the configuration thusfar.

At this instant bazel {build,test} //... doesn't include the gazelle plugin, but I would advocate that is incorrect. I don't know enough about bzlmod, but it feels like that should be correctable, am I mistaken?

@fmeum
Copy link
Contributor

fmeum commented Jan 17, 2023

I agree with @achew22 here, having the canonical Gazelle plugin right next to the ruleset code seems right to me.

At this instant bazel {build,test} //... doesn't include the gazelle plugin, but I would advocate that is incorrect. I don't know enough about bzlmod, but it feels like that should be correctable, am I mistaken?

This could be achieved by using the integration test helpers used in rules_python.

@tetromino
Copy link
Collaborator Author

This could be achieved by using the integration test helpers used in rules_python.

@fmeum, could you point me to those helpers? As far as I can see, rules_python has its rules and gazelle plugin in one workspace.

@fmeum
Copy link
Contributor

fmeum commented Jan 18, 2023

https://github.com/bazelbuild/rules_python/blob/main/tools/bazel_integration_test/bazel_integration_test.bzl is the trick I was referring to. But a separate CI job with working_directory set to a subdirectory also should make for a decent dev experience.

@shs96c
Copy link
Contributor

shs96c commented Jan 18, 2023

For context, one reason why the plugin is in its own module is that it requires the use of rules_go, and that's an expensive dependency to take since it downloads an entire Go toolchain when the module is initialised. Another may be that this prevents some nasty circular dependencies (skylib is pretty foundational), but that wasn't the motivation for creating a new module.

To go on the record, I agree with @achew22's comment.

@fmeum
Copy link
Contributor

fmeum commented Jan 18, 2023

The issue with rules_go eagerly loading toolchains has now been fixed. Depending on rules_go should now be very cheap, but still isn't right for something as foundational as bazel_skylib.

@tetromino
Copy link
Collaborator Author

tetromino commented Jan 27, 2023

Thank you for your comments - you have convinced me that splitting the plugin into a new repo is not the way to go. And we figured out a way to reasonably distribute both modules from the same git repo (#424, #429).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants