Open zx2c4 opened 5 years ago
/cc @bcmills
Should this have the release-blocker
tag? It's certainly a regression from 1.11 and 1.12.
@bcmills @jayconrod I think this is a side-effect of changing GOPROXY.
$ GOOS=windows GOPROXY=direct GONOSUM=.* go1.13beta get golang.zx2c4.com/wireguard/windows@pkg/walk go: finding golang.zx2c4.com/wireguard/windows pkg/walk
The problem can be reproducible with go1.12 if GOPROXY is set.
GOOS=windows GOPROXY=https://proxy.golang.org GONOSUM=.* go get golang.zx2c4.com/wireguard/windows@pkg/walk go: finding golang.zx2c4.com/wireguard/windows pkg/walk go: finding golang.zx2c4.com/wireguard pkg/walk go: finding golang.zx2c4.com pkg/walk go get golang.zx2c4.com/wireguard/windows@pkg/walk: disallowed version string "pkg/walk"
Such version strings are not accepted by golang.org/x/mod/module package that is used by both go command and proxy.golang.org as well. The module proxy protocol doc (go help goproxy
) does not specify an acceptable set of version strings.
@zx2c4, can you confirm that setting GOPROXY=direct
continues to allow the branch-names in question? (That probably determines whether this gets the release-blocker
label.)
This might be a limitation in the proxy, rather than the Go command.
With go1.13beta1, the command below succeeds:
$ GOPROXY=direct go mod download -json golang.zx2c4.com/wireguard/windows@pkg/walk
However, it fails with GOPROXY=https://proxy.golang.org/
. Specifically, this query fails:
$ curl https://proxy.golang.org/golang.zx2c4.com/wireguard/windows/@v/pkg/walk.info
bad request: invalid escaped version "pkg/walk": invalid char '/'
When the proxy is enabled, this is the first request the Go command will send in order to find out the canonical version for pkg/walk
.
@zx2c4, can you confirm that setting
GOPROXY=direct
continues to allow the branch-names in question? (That probably determines whether this gets therelease-blocker
label.)
Set GOPROXY=direct
works for me:
cuonglm :: /tmp/test » GOPROXY=direct go mod tidy
golang.zx2c4.com/wireguard/windows pkg/walk
go: finding golang.zx2c4.com/wireguard/windows pkg/walk
golang.zx2c4.com/wireguard/windows pkg/walk-win
go: finding golang.zx2c4.com/wireguard/windows pkg/walk-win
@hyangah
Such version strings are not accepted by golang.org/x/mod/module package
The general solution here may be for the proxy to treat any (non-pseudo-) version that fails module.Check
as a potential branch name.
@bcmills @jayconrod
Note that the same code is being used in the go command.
The golang.org/x/mod/module
package is a copy of src/cmd/go/internal/module
package. The error message users observed is probably before hitting the network. (src/cmd/go/internal/modfetch/proxy.go
)
@rsc
It wasn't clear to me, so for the record: the problem is that the module.Escape/UnescapeVersion
functions, which are only used on the proxy code paths in the go
command, don't allow slashes. So, yes, proxy.golang.org
doesn't support them, but more importantly, the go
command never even gets that far because it can't encode the request properly.
@zx2c4, can you confirm that setting GOPROXY=direct continues to allow the branch-names in question?
Confirmed.
Looking at the above, there appears to be some kind of bizarre tug-a-war with the milestone tags. I assume this is 1.13 material, considering this is some form of regression and we're in the beta period where we're supposed to be investigating those.
@zx2c4, @andybons is in the process of re-milestoning all of the non-release-blocking Go1.13 issues and this probably got swept up in a search. Probably this should be release-blocking, and then it won't get caught in that query. 🙂
Thinking about this some more: even if the proxy path in the go
command is fundamentally broken, and even if the fix for that bug is not small enough for Go 1.13, there is a simple fix within the proxy itself: specifically, to serve a 404 or 410 (“I can't help you with that.”) instead of a 400 (“Nobody can help you with that.”)
Retitling to accurately reflect the immediate fix. Once that's addressed, we can look in greater depth at the go
command's proxy path in general (probably for 1.14, but I'll see if @jayconrod or @rsc have any ideas that we could implement in the shorter term).
Note: the go command failed before sending a request to proxy.
On Tue, Jul 9, 2019, 6:34 PM Bryan C. Mills notifications@github.com wrote:
Thinking about this some more: even if the proxy path in the go command is fundamentally broken, and even if the fix for that bug is not small enough for Go 1.13, there is a simple fix within the proxy itself: specifically, to serve a 404 or 410 (“I can't help you with that.”) instead of a 400 (“Nobody can help you with that.”)
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/32955?email_source=notifications&email_token=ABGESL2F6BHDGULN464FMT3P6UG5RA5CNFSM4H6KJO22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZRXJKA#issuecomment-509834408, or mute the thread https://github.com/notifications/unsubscribe-auth/ABGESL3VR3Z3GCVOMDJ27HTP6UG5RANCNFSM4H6KJO2Q .
Ah, right. Got it. Sorry for my confusion!
This has an interesting secondary interaction with proxy fallback. Since we can't ask the proxy about the requested version string at all, we don't know whether the proxy would have rejected that version, so we can't safely proceed to direct
in the fallback list (https://github.com/golang/go/issues/31913#issuecomment-490507980).
Change https://golang.org/cl/185598 mentions this issue: cmd/go/internal/modfetch: treat invalid version strings as "not found" in (*proxyRepo).Stat
The fix is complicated and the workaround of an explicit GOPROXY=direct
continues to work, so unlabeling as release-blocker.
Thanks @bcmills.
I hoped to see the module
package and the protocol accept wider variety of version strings rather than workaround. The vcs branch, tag names may have a wider range of characters than the encoding/decoding functions in module
package would assume.
I'm hoping we can widen the accepted strings too, but I think any change to the escaping logic is probably too subtle to make 1.13.
I agree that changes here are too subtle for Go 1.13. For Go 1.14 the first question is "should we make direct mode and proxy mode match as far as what they accept for versions" and the second is "if so, how? restrict direct mode or loosen proxy mode?"
Are slashes in git branch names very common?
Are slashes in git branch names very common?
Yes, extremely common. For example, some projects use it to denote the owner of the branch jd/something
. Others use it for particular version partions. One of the most common ways of serving git repos to groups is a piece of software called gitolite
, perhaps you've heard of it. One of its key features is that it can give granular permissions to branches based on a / hierarchy. This in turn matches the actual way git stores tags: in a directory hierarchy, with / being the directory separator.
fyi - proxy.golang.org now serves 404/410 for this
$ curl -i https://proxy.golang.org/golang.zx2c4.com/wireguard/windows/@v/pkg/walk.info HTTP/1.1 410 Gone Content-Type: text/plain; charset=UTF-8 ... bad request: invalid escaped version "pkg/walk": invalid char '/'
This bug also affects module imports from a subdirectory of another module.
Here's an example tree for a repo:
github.com/foo/bar
├── pkg
│ └── baz
│ └── go.mod
└── go.mod
If I were to import package github.com/foo/bar/pkg/baz
then go would try to fetch tag pkg/baz/v1.0.0
automatically (that would fail).
What're the next steps for that one - do we want to fix go
or proxy (athens
)?
@utrack, the versions in the proxy protocol are not the same as the tags in the underlying repo. The tag for the module in the pkg/baz
subdirectory is indeed pkg/baz/v1.0.0
, but once the module is extracted into the module cache that version is encoded as simply v1.0.0
(because the pkg/baz
portion is already encoded in the module cache).
@utrack try the -x
flag to see how the go
command works with the proxy. That will show the behavior @bcmills described above. go get github.com/foo/bar/pkg/baz@v1.0.0
or go get github.com/foo/bar/pkg/baz
will work as expected.
go get github.com/foo/bar/pkg/baz@pkg/baz/v1.0.0
won't due to this issue but not sure how often ppl want to fetch a module/package in that way.
Using the hash of the branch git rev-parse
did it for me, see the comment: https://github.com/golang/go/issues/34175#issuecomment-529502530
Are slashes in git branch names very common?
As much as I don't personally use them, I'm afraid they are common. Where I work, pretty much everyone works in branches like feat/some-feature
or fix/some-bug
. For example: https://github.com/ipfs/go-ipfs/branches
To add to my last comment, the convention is even documented here: https://github.com/ipfs/community/blob/master/CONTRIBUTING_GO.md#branch-names
If you are working on a new feature, prefix your branch name with
feat/
. If you are fixing an issue,fix/
. If you are simply adding tests,test/
. If you are adding documentation,doc/
. If your changeset doesn't fall into one of these categories, use your best judgement and come up with your own short prefix.
+1 fix this
Any chance that this gets fixed at any point? Slashes in branch names are very common and it's frustrating to have to work around this limitation all the time.
I am building a main
app that will optionally support one or more database drivers. I wonder if I could branch each type of database driver separately and allow users to go get/install from respective branches if they only need one database type.
package main
import (
// ...
_ "github.com/go-sql-driver/mysql"
_ "github.com/jackc/pgx/v5/stdlib"
_ "github.com/mattn/go-sqlite3"
_ "github.com/microsoft/go-mssqldb"
_ "github.com/sijms/go-ora/v2"
_ "modernc.org/sqlite"
)
It seems it works as of go 1.20.1.
I created several branches, all
, mysql
, sqlite3
, and etc., loading only corresponding drivers. And I was able to install the different branches by:
$ go install github.com/elgs/gosqlapi@mysql
$ go install github.com/elgs/gosqlapi@sqlite3
$ go install github.com/elgs/gosqlapi@all
I am quite satisfied as it is. But I am not sure if I overlooked anything.
@elgs this issue is about interacting with branches that have a forward slash (/
) in them - e.g. branch/name
.
Setting GOPROXY='https://proxy.golang.org|direct'
can also be used as a workaround instead of the default GOPROXY=https://proxy.golang.org,direct
. This means for any error from the first URL, fall back to direct as per go1.15 release notes
replace github.com/kopia/kopia => github.com/project-velero/kopia v0.17.0-velero-patch
This with or without direct goproxy also result in
GOPROXY=direct go mod tidy
go: downloading github.com/project-velero/kopia v0.17.0-velero-patch
go: github.com/openshift/oadp-operator imports
k8s.io/apimachinery/pkg/apis/meta/v1/unstructured tested by
k8s.io/apimachinery/pkg/apis/meta/v1/unstructured.test imports
github.com/stretchr/testify/assert: github.com/kopia/kopia@v0.16.0 (replaced by github.com/project-velero/kopia@v0.17.0-velero-patch): verifying go.mod: github.com/project-velero/kopia@v0.17.0-velero-patch/go.mod: reading https://sum.golang.org/lookup/github.com/project-velero/kopia@v0.17.0-velero-patch: 404 Not Found
For Go 1.11 and 1.12, I have in my
go.mod
something like:I use this to indicate that usage of
github.com/lxn/walk
should be replaced with the latest contents of a branch calledpkg/walk
fromgolang.zx2c4.com/wireguard/windows
.With 1.13beta1, this now breaks with a message:
This poses a significant problem.