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

Allow gazelle to be aware of go_prefix mappings defined in the workspace. #313

Closed
wlynch opened this issue Mar 16, 2017 · 3 comments
Closed

Comments

@wlynch
Copy link
Contributor

wlynch commented Mar 16, 2017

Summary

Gazelle currently does not handle remote dependencies with a go_prefix value that is not URL-like. For example, if a remote dep has a go_prefix of "foo", gazelle currently assumes that this package is a part of the standard library and should not try and resolve the dep.

Proposal

Allow gazelle to be aware of go_prefix mappings defined in the workspace.

If gazelle encounters an import path matching this prefix, it should use this when it generates the deps. A prerequisite of this is to allow fetching of repos using non-URL like import paths.

Phase 1: Modify fetch_repo to take in remote and import path.

If given remote, assume that this is the exact URL and we shouldn't modify it.

This allows fetch_repo and go_repository to work on non-standard URL paths if the user specifies it explicitly. This has a few benefits:

  1. This brings the use of remote in line with git_repository.
  2. This makes go_repository rules easier to work with for private repositories that don't expose go-import meta tags. Currently teams get around this by using the new git_repository rule instead.

Phase 2: Make Gazelle aware of WORKSPACE files.

Gazelle already finds the the WORKSPACE file when running to determine the repo root. We should use the WORKSPACE to map go_prefixes to deps. If gazelle finds an import path that matches something in this map, then it uses that dep in its output. If gazelle cannot find a match, then it will default to the existing resolve_external behavior.

In the sandbox use case of new_go_repository where gazelle may not have access to the WORKSPACE file, the existing behavior should be sufficient. This is because you generally use new_go_repository because the remote dep has no bazel build files, therefore they won't override their default go_prefix, and their deps will also follow standard URL-like go import paths (following go get behavior). From an implementation standpoint, we shouldn't need to make any changes for this behavior since the dep override map described above should be empty.

Examples

fetch_repo

go_repository(
    name = "com_github_wlynch_bazeltest",
    commit = "c4501f6626eedf2b040a2c5b8bfd0c30ef4e485b",
    importpath = "wlynch/bazeltest",
    remote = "https://github.com/wlynch/bazeltest"
)
$ fetch_repo 
  --remote = "https://github.com/wlynch/bazeltest" # note: different behavior than before
  --rev = "c4501f6626eedf2b040a2c5b8bfd0c30ef4e485b"
  --importpath = "wlynch/bazeltest"
  --dest = ctx.path('')

fetch_repo sees that remote is explicitly set. It skips the vcs.RepoRootForImportPath() step (which is used for figuring out the repo URL from the import path), assuming that the user has provided a correct URL. We continue on as normal, shelling out to git and fetch the repository.

gazelle

import(
  "wlynch/bazeltest"
)
$ gazelle
  1. Find WORKSPACE, parse all *go_repository rules
  2. Keep map of explicit importpath -> name mappings (e.g. "wlynch/bazeltest" => "com_github_wlynch_bazeltest")
    • In practice, this should probably be a tree keyed on the path- e.g. {"wlynch": {"bazeltest": "com_github_wlynch_bazeltest"}}
  3. Parse go file, see if "wlynch/bazeltest" see that this matches "com_github_wlynch_bazeltest".
    • With the tree, this would be: see if "wlynch" exists, get subtree. see if "bazeltest" exists, get "com_github_wlynch_bazeltest".
  4. Match is found, use "@com_github_wlynch_bazeltest//" as the dep root (appending the remaining path as necessary).
@wlynch wlynch self-assigned this Mar 16, 2017
@pmbethe09
Copy link
Member

Overall SGTM.

@jayconrod
Copy link
Contributor

SGTM too.

wlynch added a commit to wlynch/rules_go that referenced this issue Mar 20, 2017
This change allows fetch_repo to fetch repositories without attempting
to determine the repo root if the user supplies the information
themselves. This allows users to be able to fetch repositories that the
vcs package would otherwise not be able to recognize.

This change also changes the behavior of the --remote flag to match the
git_repository rule.
wlynch added a commit to wlynch/rules_go that referenced this issue Mar 20, 2017
This change allows fetch_repo to fetch repositories without attempting
to determine the repo root if the user supplies the information
themselves. This allows users to be able to fetch repositories that the
vcs package would otherwise not be able to recognize.

This change also changes the behavior of the --remote flag to match the
git_repository rule.
wlynch added a commit to wlynch/rules_go that referenced this issue Mar 20, 2017
This change allows fetch_repo to fetch repositories without attempting
to determine the repo root if the user supplies the information
themselves. This allows users to be able to fetch repositories that the
vcs package would otherwise not be able to recognize.

This change also changes the behavior of the --remote flag to match the
git_repository rule.
jayconrod pushed a commit that referenced this issue Mar 21, 2017
* go_repository: add importpath and vcs flags (#313)

This change allows fetch_repo to fetch repositories without attempting
to determine the repo root if the user supplies the information
themselves. This allows users to be able to fetch repositories that the
vcs package would otherwise not be able to recognize.

This change also changes the behavior of the --remote flag to match the
git_repository rule.

* Respond to jayconrod@ comments in #319.

* Add BUILD rule for fetch_repo_test.go
* Stub out vcs.RepoRootForImportPath
* Make remote and vcs flags not being set in fetch_repo a hard error.
* Verify VCS is not nil before returning it in a RepoRoot.

* Fix go_repository rule for #319.

Since the vcs field in go_repository was always set to "git", fetch_repo was
always failing if the user passed in only the importpath.
jayconrod pushed a commit to jayconrod/rules_go that referenced this issue Apr 7, 2017
…ild#319)

* go_repository: add importpath and vcs flags (bazelbuild#313)

This change allows fetch_repo to fetch repositories without attempting
to determine the repo root if the user supplies the information
themselves. This allows users to be able to fetch repositories that the
vcs package would otherwise not be able to recognize.

This change also changes the behavior of the --remote flag to match the
git_repository rule.
jayconrod pushed a commit to jayconrod/rules_go that referenced this issue Apr 7, 2017
…ild#319)

* go_repository: add importpath and vcs flags (bazelbuild#313)

This change allows fetch_repo to fetch repositories without attempting
to determine the repo root if the user supplies the information
themselves. This allows users to be able to fetch repositories that the
vcs package would otherwise not be able to recognize.

This change also changes the behavior of the --remote flag to match the
git_repository rule.

* Respond to jayconrod@ comments in bazelbuild#319.

* Add BUILD rule for fetch_repo_test.go
* Stub out vcs.RepoRootForImportPath
* Make remote and vcs flags not being set in fetch_repo a hard error.
* Verify VCS is not nil before returning it in a RepoRoot.

* Fix go_repository rule for bazelbuild#319.

Since the vcs field in go_repository was always set to "git", fetch_repo was
always failing if the user passed in only the importpath.
@jayconrod
Copy link
Contributor

Closing old Gazelle issues. I've migrated the remaining work to bazelbuild/bazel-gazelle#6. Let me know if I missed anything.

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