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

cmd/go: legacy dependency conversion bypasses GOPROXY #33767

Open
aofei opened this issue Aug 21, 2019 · 8 comments

Comments

@aofei
Copy link
Contributor

commented Aug 21, 2019

Well, one of my friends (@EDDYCJY) tried to migrate a project from their company from golang/dep to Go modules. When he executes go mod init example.com/foo/bar, go parses the Gopkg.lock file, which causes the initialization to fail because the endpoints of some of the modules in Gopkg.lock are blocked by their company's firewall. We thought the firewall problem could be solved by setting GOPROXY until I found modfetch.ImportRepoRev:

// ImportRepoRev returns the module and version to use to access
// the given import path loaded from the source code repository that
// the original "go get" would have used, at the specific repository revision
// (typically a commit hash, but possibly also a source control tag).
func ImportRepoRev(path, rev string) (Repo, *RevInfo, error) {
if cfg.BuildMod == "vendor" || cfg.BuildMod == "readonly" {
return nil, nil, fmt.Errorf("repo version lookup disabled by -mod=%s", cfg.BuildMod)
}
// Note: Because we are converting a code reference from a legacy
// version control system, we ignore meta tags about modules
// and use only direct source control entries (get.IgnoreMod).
security := web.SecureOnly
if get.Insecure {
security = web.Insecure
}
rr, err := get.RepoRootForImportPath(path, get.IgnoreMod, security)
if err != nil {
return nil, nil, err
}
code, err := lookupCodeRepo(rr)
if err != nil {
return nil, nil, err
}
revInfo, err := code.Stat(rev)
if err != nil {
return nil, nil, err
}
// TODO: Look in repo to find path, check for go.mod files.
// For now we're just assuming rr.Root is the module path,
// which is true in the absence of go.mod files.
repo, err := newCodeRepo(code, rr.Root, rr.Root)
if err != nil {
return nil, nil, err
}
info, err := repo.(*codeRepo).convert(revInfo, rev)
if err != nil {
return nil, nil, err
}
return repo, info, nil
}

I also found that modfetch.ImportRepoRev first appeared in the golang/vgo@97ff4ad submitted by @rsc. And the reason is:

  • Add ImportRepoRev, because neither Lookup nor Import
    is appropriate for translating legacy versioning system configs.
    There's more to do in ImportRepoRev, but it's already more
    accurate than before.

I investigated it as much as I can. Then I think the "inappropriate" that @rsc said before may have gone now, since modload.Query is now very powerful. So I added support for the module proxy for modfetch.ImportRepoRev by simply doing something like modload.Query.

I just submitted a patch. If the patch can be merged, then I suggest applying those changes to Go 1.13 since it has not yet been released. 😊

@gopherbot

This comment has been minimized.

Copy link

commented Aug 21, 2019

Change https://golang.org/cl/191218 mentions this issue: cmd/go: add module proxy support for modfetch.ImportRepoRev``

@bcmills bcmills changed the title cmd/go: add module proxy support for `modfetch.ImportRepoRev` cmd/go: Gopkg.lock conversion bypasses GOPROXY Aug 22, 2019

@bcmills bcmills added this to the Go1.14 milestone Aug 22, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

I suggest applying those changes to Go 1.13 since it has not yet been released. 😊

Go 1.13 is well into the release freeze at this point, so we're pretty much only fixing critical regressions from here on out. The good news is, the tree will be opening for 1.14 very soon.

@aofei

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

And this problem isn't just for Gopkg.lock. It exists when translating legacy versioning system configs. So the title should be corrected.

@aofei aofei changed the title cmd/go: Gopkg.lock conversion bypasses GOPROXY cmd/go: legacy dependency conversion bypasses GOPROXY Aug 22, 2019

@aofei

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

Hi @bcmills, someone has reported this bug to me (again) today. This bug isn't only triggered when go mod init is called, but in every operation (such as go test, go run, go build etc) that might generate a go.mod file from a legacy dependency (when GO111MODULE=on). The people who have encountered this bug are blaming their Go module proxies (at least goproxy.cn was blamed by someone). They think that Go module proxy should be able to proxy this kind of initialization operation.

So, please merge https://golang.org/cl/191218 and consider applying those changes to Go 1.13.1 (or 1.13.2). We really need it. 🙏

Thanks again. ❤️

@gopherbot gopherbot closed this in 04867cd Sep 11, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

CL 191218 is in, although I don't think it meets the backport criteria (it wasn't a regression and the workaround is to run go mod tidy after go mod init).

Regarding other operations that might generate a go.mod file: they no longer do as of 1.13. (Only go mod init creates the go.mod file now.) That was #29433.

@aofei

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

Alright, I got it. Thank you for your reply. :-)

@bcmills bcmills reopened this Sep 16, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

This was reverted in CL 194797 due to a test failure, possibly related to #34092. Investigating now.

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

The reason for the test failure seems to be that some of the converted formats specify package paths rather than module paths, and modfetch.Stat does not know how to resolve a package path to a module.

The usual function that converts a package path to a module path is modload.QueryPackage, but we can't call that within modconv due to a circular dependency.

The circular dependency exists because modload is currently performing (at least) two different roles:

  1. Managing the contents of the main module's build list, and
  2. managing the mapping of module paths and versions to source file locations.

Role (2) will need significant refactoring for #26904 anyway, so I'm planning to revisit this once that has been addressed.

CC @jayconrod

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.