-
Notifications
You must be signed in to change notification settings - Fork 380
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
Use package name instead of go_default_library for rules #5
Comments
I think you meant to say: at |
Correct. I've edited the original issue. |
Just wonder if there's any ETA when this would be fixed. Thanks! |
No ETA, sorry. |
This would need a major migration plan to become effective, right? |
@riking Yes |
Is there an active design document for this, or should one be drafted? Is there a consensus? I've read a few different assertions on how things should work that all seem to slightly contradict each other. I'm rolling out gazelle to our monorepo, and the "go_default_library" everywhere can be a little grating on the eye. |
Sorry, this is annoying. I'm afraid there's no active plan to make this happen at the moment though. Other functional issues (editor support, coverage, proto support) still seem more important. The problem is migration. A change to all rules will be really disruptive for large projects (and people have objected loudly to disruptions of this scale in the past). So this definitely needs to be opt-in for existing projects. At the same time, the new names should be used by default for new projects. This is also difficult for repositories that are used as dependencies in other repositories. If they want to switch to the new names, they'd need to have aliases with the old names to avoid breaking their dependents. Gazelle should be able to generate those names. |
Would it be possible to draft the design without executing on it? I can create a design doc if you want. That way we know where we are headed. What I'd need is an initial summary of what you would like to see, plus a list of considerations similar to the above message. We have our own little monorepo over here, and I am already half inclined to hack gazelle (which I tried yesterday for verification purposes) to produce better names before I start enforcing it via a glaze-style presubmit. It would be nice to know that the solution is compatible with the desired end state, and if desired I could contribute it behind said opt-in flag. |
Here's a sketch of how I think this should work. I'd rather not review a more detailed design doc right now though.
|
EDIT: These questions were pretty much answered during implementation. Ignore the below. For Eg. package '//foo' with importpath 'bar'. Should the |
Add directive 'go_naming_convention' to control the name of generated Go rules. go_default_library: Legacy behaviour. import: Name targets after their import path. import_alias: Same as import, but generate alias targets to maintain backwards compatibility with go_default_library. We also add a `build_naming_convention` attribute to `go_repository` rules, allowing per-external control over the naming convention. gazelle fix is augmented to migrate between the different styles, both forwards and backwards. FIXES=bazel-contrib#5
Add directive 'go_naming_convention' to control the name of generated Go rules. go_default_library: Legacy behaviour. import: Name targets after their import path. import_alias: Same as import, but generate alias targets to maintain backwards compatibility with go_default_library. We also add a `build_naming_convention` attribute to `go_repository` rules, allowing per-external control over the naming convention. gazelle fix is augmented to migrate between the different styles, both forwards and backwards. FIXES=bazel-contrib#5
Add directive 'go_naming_convention' to control the name of generated Go rules. go_default_library: Legacy behaviour. import: Name targets after their import path. import_alias: Same as import, but generate alias targets to maintain backwards compatibility with go_default_library. We also add a `build_naming_convention` attribute to `go_repository` rules, allowing per-external control over the naming convention. gazelle fix is augmented to migrate between the different styles, both forwards and backwards. FIXES=bazel-contrib#5
Add directive 'go_naming_convention' to control the name of generated Go rules. go_default_library: Legacy behaviour. import: Name targets after their import path. import_alias: Same as import, but generate alias targets to maintain backwards compatibility with go_default_library. We also add a `build_naming_convention` attribute to `go_repository` rules, allowing per-external control over the naming convention. gazelle fix is augmented to migrate between the different styles, both forwards and backwards. FIXES=bazel-contrib#5
Add directive 'go_naming_convention' to control the name of generated Go rules. go_default_library: Legacy behaviour. import: Name targets after their import path. import_alias: Same as import, but generate alias targets to maintain backwards compatibility with go_default_library. We also add a `build_naming_convention` attribute to `go_repository` rules, allowing per-external control over the naming convention. gazelle fix is augmented to migrate between the different styles, both forwards and backwards. FIXES=bazel-contrib#5
Add directive 'go_naming_convention' to control the name of generated Go rules. go_default_library: Legacy behaviour. import: Name targets after their import path. import_alias: Same as import, but generate alias targets to maintain backwards compatibility with go_default_library. We also add a `build_naming_convention` attribute to `go_repository` rules, allowing per-external control over the naming convention. gazelle fix is augmented to migrate between the different styles, both forwards and backwards. FIXES=bazel-contrib#5
@linzhp @blico Just wanted to give you a heads up that @tomlu is working on a fix for this issue (#808). The plan we're following is basically this comment above. Please let me know if there's something we're not anticipating though. |
Add directive 'go_naming_convention' to control the name of generated Go rules. go_default_library: Legacy behaviour. import: Name targets after their import path. import_alias: Same as import, but generate alias targets to maintain backwards compatibility with go_default_library. We also add a `build_naming_convention` attribute to `go_repository` rules, allowing per-external control over the naming convention. gazelle fix is augmented to migrate between the different styles, both forwards and backwards. For #5
* For external dependency resolution, if we don't know what naming convention the external repository uses (for example, because there's no known repository), we'll now use goDefaultLibraryNamingConvention. This avoids assuming that repositories with build files fetched with http_archive have been updated. * In migrateNamingConvention, print a warning and avoid renaming if there's already a target with the new name. * Library, test, and alias actuals are now generated based on import path instead of package name. * Added unknownNamingConvention, a new zero value. * Added detectNamingConvention. It reads the root build file and build files in subdirectories one level deep to infer the naming convention used if one is not specified in the root build file. Defaults to importNamingConvention. * Fixed tests. For bazel-contrib#5
This fixes an issue with importing bazel-skylib into google3. Currently, Glaze (internal Go build file generator) attempts to generate a target (//gazelle:gazelle) that conflicts with one that's already declared here. I think the right solution is actually to move the package into a subdirectory. In the future (bazel-contrib/bazel-gazelle#5), Gazelle's Go extension will generate target names similar to what Glaze does, so the same conflict will happen in open source. I think it's also logical to have a directory of packages in case more need to be added in the future, and for the extension to have a package name matching the language it works with. This is an incompatible change, but the //gazelle directory isn't part of a tagged release yet, so hopefully it won't break anyone.
* For external dependency resolution, if we don't know what naming convention the external repository uses (for example, because there's no known repository), we'll now use goDefaultLibraryNamingConvention. This avoids assuming that repositories with build files fetched with http_archive have been updated. * In migrateNamingConvention, print a warning and avoid renaming if there's already a target with the new name. * Library, test, and alias actuals are now generated based on import path instead of package name. * Added unknownNamingConvention, a new zero value. * Added detectNamingConvention. It reads the root build file and build files in subdirectories one level deep to infer the naming convention used if one is not specified in the root build file. Defaults to importNamingConvention. * Delete empty go_library rules in directories with go_binary. * Fixed tests. For #5
* Set naming convention in main build file. * Add gazelle:repository declarations in WORKSPACE for repos in gazelle_dependencies. * Run 'gazelle fix'. * Manual fixes for manually written targets. For #5
Allows the user to set the default Go naming convention used when resolving imports of packages in external repositories with unknown naming conventions. For bazel-contrib#5
Allows the user to set the default Go naming convention used when resolving imports of packages in external repositories with unknown naming conventions. For #5
* Rename goNamingConventionExtern to goNamingConventionExternal, together with the flag and directive. No need to abbrev. * Only migrate go_library if it matches the expected import path for the directory. This avoids migrating the wrong target when there are multiple go_libraries in the same directory. * Only migrate go_test if it embeds the library. * When fixing rules, don't add an embed attribute to go_test. For bazel-contrib#5
* Rename goNamingConventionExtern to goNamingConventionExternal, together with the flag and directive. No need to abbrev. * Only migrate go_library if it matches the expected import path for the directory. This avoids migrating the wrong target when there are multiple go_libraries in the same directory. * Only migrate go_test if it embeds the library. * When fixing rules, don't add an embed attribute to go_test. For #5
With this change, Gazelle supports multiple naming conventions for Go targets. In new projects, new targets will be named after their import paths by default. Existing projects can continue to use their current naming convention (which will be detected automatically in most cases), or they may migrate by adding this directive to their root build file, then running Gazelle. # gazelle:go_naming_convention import go_repository uses the import_alias convention by default, which generates targets using the import convention and aliases using the go_default_library convention. Thanks to @tomlu for implementing nearly all of this! Fixes #5
* Move Gazelle extension to //gazelle/bzl and change package name This fixes an issue with importing bazel-skylib into google3. Currently, Glaze (internal Go build file generator) attempts to generate a target (//gazelle:gazelle) that conflicts with one that's already declared here. I think the right solution is actually to move the package into a subdirectory. In the future (bazel-contrib/bazel-gazelle#5), Gazelle's Go extension will generate target names similar to what Glaze does, so the same conflict will happen in open source. I think it's also logical to have a directory of packages in case more need to be added in the future, and for the extension to have a package name matching the language it works with. This is an incompatible change, but the //gazelle directory isn't part of a tagged release yet, so hopefully it won't break anyone. * fix runfiles access in test * Fix gazelle package names. Co-authored-by: Jay Conrod <[email protected]>
* Move Gazelle extension to //gazelle/bzl and change package name This fixes an issue with importing bazel-skylib into google3. Currently, Glaze (internal Go build file generator) attempts to generate a target (//gazelle:gazelle) that conflicts with one that's already declared here. I think the right solution is actually to move the package into a subdirectory. In the future (bazel-contrib/bazel-gazelle#5), Gazelle's Go extension will generate target names similar to what Glaze does, so the same conflict will happen in open source. I think it's also logical to have a directory of packages in case more need to be added in the future, and for the extension to have a package name matching the language it works with. This is an incompatible change, but the //gazelle directory isn't part of a tagged release yet, so hopefully it won't break anyone. * fix runfiles access in test * Fix gazelle package names. Co-authored-by: Jay Conrod <[email protected]>
Originally bazel-contrib/rules_go#265
We used
go_default_library
as the name forgo_library
rules generated by Gazelle to simplify dependency resolution. Both the Go rules and Gazelle relied on this, along withgo_prefix
.Now,
go_prefix
is deprecated and Gazelle addsimportpath
attributes to all Go rules. It also uses an index for dependency resolution instead of transforming import path strings. There's no longer a technical need forgo_default_library
, so we'd like to migrate away from that name.In general, we'd like the library name to match the last component of the Bazel package. For example, a library stored at
//foo/bar
should be namedbar
, so it can be compiled withbazel build //foo/bar
instead ofbazel build //foo/bar:go_default_library
. Tests would be namedbar_test
orbar_xtest
.We may want this to work differently in packages with a
go_binary
(sources havepackage main
). For these packages, thego_binary
name should match the package (again, so it can be compiled with//foo/bar
). The library will need a different name. We may want to put sources and dependencies ingo_binary
instead of embedding a library if there are no tests.The text was updated successfully, but these errors were encountered: