support sub-path in goinstall:// (implement same behaviour as go buid)#266
support sub-path in goinstall:// (implement same behaviour as go buid)#266gebi wants to merge 2 commits into
Conversation
implement same behaviour as go build to find base module path. Sadly https://pkg.go.dev/golang.org/x/tools/go/vcs is deprecated thus this is implemented as recommended via call to `go list`
baseModulePath needs to be called without version
| for len(parts) > 0 { | ||
| mod := strings.Join(parts, "/") | ||
| out, err := exec.Command("go", "list", "-m", "-f", "{{.Path}}", mod+"@latest").Output() | ||
| if err == nil { |
There was a problem hiding this comment.
what happens if go list fails for other error than "no matching version". Error handling seems it needs improvement.
There was a problem hiding this comment.
you are right, if the PR is "ok" in principle i'd add unit tests too, because the behaviour currently expected by the call to go list should be tested too.
| parts := strings.Split(noVer, "/") | ||
| for len(parts) > 0 { | ||
| mod := strings.Join(parts, "/") | ||
| out, err := exec.Command("go", "list", "-m", "-f", "{{.Path}}", mod+"@latest").Output() |
There was a problem hiding this comment.
I'd really prefer avoiding having to do this. Is this how go install actually does it?
There was a problem hiding this comment.
i think so, yes...
% go install -x github.com/mgit-at/godeb/cmd/godeb@latest
# get https://proxy.golang.org/github.com/@v/list
# get https://proxy.golang.org/github.com/mgit-at/godeb/cmd/@v/list
# get https://proxy.golang.org/github.com/mgit-at/godeb/@v/list
# get https://proxy.golang.org/github.com/mgit-at/@v/list
# get https://proxy.golang.org/github.com/mgit-at/godeb/cmd/godeb/@v/list
# get https://proxy.golang.org/github.com/mgit-at/godeb/@v/list: 200 OK (0.028s)
# get https://proxy.golang.org/github.com/mgit-at/godeb/@latest
# get https://proxy.golang.org/github.com/mgit-at/godeb/cmd/@v/list: 404 Not Found (0.052s)
# get https://proxy.golang.org/github.com/@v/list: 404 Not Found (0.052s)
# get https://proxy.golang.org/github.com/mgit-at/godeb/cmd/godeb/@v/list: 404 Not Found (0.056s)
# get https://proxy.golang.org/github.com/mgit-at/godeb/@latest: 200 OK (0.028s)
# get https://proxy.golang.org/github.com/mgit-at/@v/list: 404 Not Found (0.057s)
WORK=/tmp/go-build3389364809
It would also be possible to call proxy.golang.org directly and implement the walking via the http api but at the expense of breaking and re-implementing the whole local/private/... module machinery that's hidden beneath go list.
sadly i could not find any other way...
the pkg holding RepoRootForImportPath as a separate and externally usable implementation for this got deprecated because of code rot https://pkg.go.dev/golang.org/x/tools/go/vcs and the only suggested solution is to call go list instead.
The real implementation can be found here vcs.RepoRootForImportPath and it's imho too complex and sensitive to extract, as it requires other packages from go/internal too so much so that even the pkg in x/tools/go gave up tracking it...
i have a implementation that parses the -x (trace) output of go list, as this tool leaks the output of vcs.RepoRootForImportPath there, which would require no recursive calls but after having done the impl it's that ugly to parse trace output and brittle i looked for other possibilities...
There was a problem hiding this comment.
i think i've an idea how to make the default case quicker and still have a somewhat sane impl, i'll cook something up and push into this PR
There was a problem hiding this comment.
sadly i could not find any other way...
the pkg holdingRepoRootForImportPathas a separate and externally usable implementation for this got deprecated because of code rot pkg.go.dev/golang.org/x/tools/go/vcs and the only suggested solution is to callgo listinstead.
btw not sure if this helps but we've recently also fixed and extracted this functionality to use it in github.com/dagger/dagger so if needed, it can be used from here: https://github.com/dagger/dagger/blob/ba4c738ed055b37f7191678f43c3436e741c910a/engine/vcs/vcs.go#L342
There was a problem hiding this comment.
nice, i've done something similar, though i extracted my base version from the current state of go/src/cmd/go/internal/vcs/vcs.go and the one in dagger seems to be the old deprecated version from x/tools/go/vcs/vcs.go.
I'd rather not touch a code that upstream deemed unmaintainable and insecure because it diverted so much that security patches can't be applied.
My idea was to implement RepoRootForImportPath via taking repoRootFromVCSPaths from current upstream and implement a stub for repoRootForImportDynamic using go list.
thus we would be shielded from much of the complexity with only the implementation of static matches/hosts (like github/gitlab/...) and one vcsPing call for verification from go and everything else goes out of process via go list.
current state is, i've a complete extract of up-to-date vcs.go running. But after doing that it's imho kinda ugly and defies the purpose of being exactly the same mess and unmaintainable over time, thus my idea from above...
in progress... https://github.com/gebi/goxt
2 participants
Hi,
this PR adds support for sub-path's in
goinstall://provider as is supported ingo installitself too.It implements the same behaviour as go build to find base module path.
Sadly https://pkg.go.dev/golang.org/x/tools/go/vcs is deprecated, thus this is implemented as recommended via call to
go list.The only real downside is, that this PR requires at least one additional external call to
go listfor every pkg that is handled viagoinstall://even if no subpath is actually used (for actual sub-paths it results in one network call per sub-path component on each invocation).But we get nice sub-path support
Without this PR the call results in an error (
go installdirectly works though)output of list cmd
bin/config.jsonlooks fineupdate / ensure / remove also works