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

Gazelle should resolve labels using declared import paths #859

Closed
jayconrod opened this issue Sep 29, 2017 · 5 comments
Closed

Gazelle should resolve labels using declared import paths #859

jayconrod opened this issue Sep 29, 2017 · 5 comments

Comments

@jayconrod
Copy link
Contributor

Currently, Gazelle converts import paths into Bazel labels by following a few simple conventions. This requires go_library to be in specific packages with specific names (go_default_library) in order to work. This is causing an increasing amount of difficulty for several reasons.

Smarter label resolution is a prerequisite for all of this.


Proposal

When Gazelle starts, it will build a table that maps import paths to labels and directories. This table will be populated by several mechanisms:

  • For external repositories that use flat build files generated by Gazelle stored in a third_party directory, Gazelle will scan these files.
  • For packages in the current repository that Gazelle won't update, Gazelle will scan existing build files.
  • For packages in the current repository that Gazelle will update, Gazelle will scan source files.

After build files and directories are scanned, Gazelle will generate rules for the packages it needs to update. Dependencies will be resolved using the table described above.

  • If an import path corresponds to one or more labels, Gazelle will choose one using vendoring logic and visibility. If there are multiple valid labels, Gazelle will emit an error.
  • If an import path points to a repository that could not be scanned for some reason, Gazelle will fall back to current conventions.
  • If an import path can't be resolved (it points to a known prefix, and there's no visible label), Gazelle will emit an error.
@ianthehat
Copy link
Contributor

"If there are multiple valid labels, Gazelle will emit an error"
Will it write out the files anyway (using the first match with a comment) so you can resolve by hand?

@jayconrod
Copy link
Contributor Author

Yes. Gazelle errors are almost never fatal. When it fails to find a label for an import path, it won't emit a label for that import. So you'll need to manually add one with a # keep comment.

@guoshimin
Copy link
Contributor

Has this proposal been accepted? Has work started? If not, what's it blocked on?

@jayconrod
Copy link
Contributor Author

I'm actively working on this on a dev branch. I'm hoping to cut the first changes this week.

jayconrod pushed a commit to jayconrod/rules_go that referenced this issue Nov 15, 2017
Walk now calls its callback in all directories it traverses, whether
or not they have buildable code or are outside of the subtrees Gazelle
was asked to update.

This is needed for import path indexing, since we'll need to scan
rules in all directories.

Related bazel-contrib#859
jayconrod added a commit that referenced this issue Nov 16, 2017
Walk now calls its callback in all directories it traverses, whether
or not they have buildable code or are outside of the subtrees Gazelle
was asked to update.

This is needed for import path indexing, since we'll need to scan
rules in all directories.

Related #859
jayconrod pushed a commit to jayconrod/rules_go that referenced this issue Nov 17, 2017
This will make it simpler to separate indexing and generating rules
and resolving dependencies into separate phases.

Related bazel-contrib#859
jayconrod added a commit that referenced this issue Nov 20, 2017
…1032)

This will make it simpler to separate indexing and generating rules
and resolving dependencies into separate phases.

Related #859
jayconrod pushed a commit to jayconrod/rules_go that referenced this issue Nov 21, 2017
Previously, rules.Generator performed import resolution (using the
resolve package) as it generated rules. With this change, Generator
now produces a "_gazelle_imports" attribute, which is resolved and
replaced with "deps".

This is necessary since we must be able to index generated rules. We
need to generate and index all rules, then resolve imports to deps in
a separate phase.

Related bazel-contrib#859
jayconrod added a commit that referenced this issue Nov 21, 2017
Previously, rules.Generator performed import resolution (using the
resolve package) as it generated rules. With this change, Generator
now produces a "_gazelle_imports" attribute, which is resolved and
replaced with "deps".

This is necessary since we must be able to index generated rules. We
need to generate and index all rules, then resolve imports to deps in
a separate phase.

Related #859
jayconrod pushed a commit to jayconrod/rules_go that referenced this issue Nov 21, 2017
(Unit tests will be in the PR to wire this into Resolver).

Related bazel-contrib#859
jayconrod added a commit that referenced this issue Nov 27, 2017
(Unit tests will be in the PR to wire this into Resolver).

Related #859
jayconrod pushed a commit to jayconrod/rules_go that referenced this issue Dec 2, 2017
We need to merge some attributes on rules before indexing them. In
particular, we need to merge "importpath" and "srcs", since these
become keys in the index. It's important to account for '# keep'
comments and '# gazelle:ignore' directives so we need to use the
regular merging algorithm instead of something ad hoc in the indexer.
However, we also need to merge deps after they're resolved.

This means that we need to merge rules in two separate phases. Right
after rules are generated, we merge everything except "deps". Then we
resolve dependencies. Then we merge "deps" only.

This should also prevent us from indexing rules that are deleted.

Related bazel-contrib#859
jayconrod added a commit that referenced this issue Dec 4, 2017
We need to merge some attributes on rules before indexing them. In
particular, we need to merge "importpath" and "srcs", since these
become keys in the index. It's important to account for '# keep'
comments and '# gazelle:ignore' directives so we need to use the
regular merging algorithm instead of something ad hoc in the indexer.
However, we also need to merge deps after they're resolved.

This means that we need to merge rules in two separate phases. Right
after rules are generated, we merge everything except "deps". Then we
resolve dependencies. Then we merge "deps" only.

This should also prevent us from indexing rules that are deleted.

Related #859
jayconrod pushed a commit to jayconrod/rules_go that referenced this issue Dec 5, 2017
@jayconrod
Copy link
Contributor Author

Closing old Gazelle issues. This is mostly done. A few remaining open issues have been migrated to bazelbuild/bazel-gazelle.

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

3 participants