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

Use package name instead of go_default_library for rules #5

Closed
jayconrod opened this issue Dec 6, 2017 · 12 comments · Fixed by #808 or #863
Closed

Use package name instead of go_default_library for rules #5

jayconrod opened this issue Dec 6, 2017 · 12 comments · Fixed by #808 or #863

Comments

@jayconrod
Copy link
Contributor

jayconrod commented Dec 6, 2017

Originally bazelbuild/rules_go#265

We used go_default_library as the name for go_library rules generated by Gazelle to simplify dependency resolution. Both the Go rules and Gazelle relied on this, along with go_prefix.

Now, go_prefix is deprecated and Gazelle adds importpath 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 for go_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 named bar, so it can be compiled with bazel build //foo/bar instead of bazel build //foo/bar:go_default_library. Tests would be named bar_test or bar_xtest.

We may want this to work differently in packages with a go_binary (sources have package main). For these packages, the go_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 in go_binary instead of embedding a library if there are no tests.

@abergmeier
Copy link

abergmeier commented Dec 20, 2017

at //foo/bar should be named foo

I think you meant to say: at //foo/bar should be named bar.
And cannot wait for this change to finally land.

@jayconrod
Copy link
Contributor Author

Correct. I've edited the original issue.

@thaidn
Copy link

thaidn commented Sep 29, 2018

Just wonder if there's any ETA when this would be fixed. Thanks!

@jayconrod
Copy link
Contributor Author

No ETA, sorry.

@riking
Copy link
Contributor

riking commented Dec 23, 2019

This would need a major migration plan to become effective, right?

@jayconrod
Copy link
Contributor Author

@riking Yes

@tomlu
Copy link
Contributor

tomlu commented May 29, 2020

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.

@jayconrod
Copy link
Contributor Author

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.

@tomlu
Copy link
Contributor

tomlu commented May 29, 2020

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.

@jayconrod
Copy link
Contributor Author

Here's a sketch of how I think this should work. I'd rather not review a more detailed design doc right now though.

  • The //language/go extension should accept a directive, # gazelle:go_naming_convention, that supports the values:
    • go_default_library - old style, what Gazelle currently does.
    • import - new style.
      • For a package whose import path ends with foo, the go_library would be named foo. The go_test would be named foo_test.
      • For a main package, the go_library would be named foo_lib, the go_test would be named foo_test, and the go_binary would be named foo.
      • Major version suffixes would be dropped from import paths for naming purposes. So a library with the path example.com/foo/v2 would be named foo, not v2.
    • import_alias - same as import but additionally generates alias rules mapping go_default_library targets to import targets.
  • If the directive isn't specified anywhere, //language/go should attempt to automatically determine the naming convention currently in use by scanning build files one or two directories deep from the root. If it doesn't find any go_library rules conforming to one style or the other (as will be the case for a repository without any build files), it should use the import style.
  • The Fix method should rename rules as needed between the two styles. That should probably happen whether or not c.ShouldFix is set, since there's explicit user intent here.
  • go_repository should have a build_naming_convention attribute, used to set this directive. It should default to import_alias.
  • External dependency resolution should respect the build_naming_convention attribute for known repositories. If the repository is unknown or if the attribute isn't set explicitly, it should follow the convention currently in use.

@tomlu
Copy link
Contributor

tomlu commented Jun 6, 2020

EDIT: These questions were pretty much answered during implementation. Ignore the below.

For import mode, what happens when the bazel package name and importpath differ?

Eg. package '//foo' with importpath 'bar'.

Should the go_library be called foo or bar? (Similar question for the go_binary)

tomlu added a commit to tomlu/bazel-gazelle that referenced this issue Jun 7, 2020
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=bazelbuild#5
tomlu added a commit to tomlu/bazel-gazelle that referenced this issue Jun 7, 2020
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=bazelbuild#5
tomlu added a commit to tomlu/bazel-gazelle that referenced this issue Jun 10, 2020
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=bazelbuild#5
tomlu added a commit to tomlu/bazel-gazelle that referenced this issue Jun 10, 2020
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=bazelbuild#5
tomlu added a commit to tomlu/bazel-gazelle that referenced this issue Jun 10, 2020
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=bazelbuild#5
tomlu added a commit to tomlu/bazel-gazelle that referenced this issue Jun 13, 2020
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=bazelbuild#5
@jayconrod
Copy link
Contributor Author

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

jayconrod pushed a commit that referenced this issue Jul 15, 2020
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
jayconrod pushed a commit to jayconrod/bazel-gazelle that referenced this issue Jul 17, 2020
* 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 bazelbuild#5
jayconrod pushed a commit to jayconrod/bazel-skylib that referenced this issue Aug 4, 2020
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 (bazelbuild/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.
jayconrod pushed a commit that referenced this issue Aug 7, 2020
* 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
jayconrod pushed a commit that referenced this issue Aug 7, 2020
* 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
jayconrod pushed a commit to jayconrod/bazel-gazelle that referenced this issue Aug 11, 2020
Allows the user to set the default Go naming convention used when
resolving imports of packages in external repositories with unknown
naming conventions.

For bazelbuild#5
jayconrod pushed a commit that referenced this issue Aug 11, 2020
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
jayconrod pushed a commit to jayconrod/bazel-gazelle that referenced this issue Aug 18, 2020
* 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 bazelbuild#5
jayconrod pushed a commit that referenced this issue Aug 18, 2020
* 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
jayconrod pushed a commit that referenced this issue Aug 19, 2020
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
c-parsons pushed a commit to bazelbuild/bazel-skylib that referenced this issue Aug 20, 2020
* 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 (bazelbuild/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 <jayconrod@google.com>
ngiloq6 added a commit to ngiloq6/bazel-skylib that referenced this issue Aug 17, 2024
* 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 (bazelbuild/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 <jayconrod@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants