cmd/go: non-top-level packages ending in "/vendor" are omitted from module zip files

[bep]

The following program fails in both go1.14rc1 and go1.13.7 darwin/amd64.

package main

import (
	"fmt"

	"github.com/bep/testmodlib"
	"github.com/bep/testmodlib/somepackage"
	"github.com/bep/testmodlib/somepackage/vendor"
	"github.com/bep/testmodlib/somepackage/vendors"
)

func main() {
	fmt.Println(testmodlib.Hello())
	fmt.Println(somepackage.Hello())
	fmt.Println(vendor.Hello())
	fmt.Println(vendors.Hello())
}

Fails with:

build github.com/bep/temp: cannot load github.com/bep/testmodlib/somepackage/vendor: module github.com/bep/testmodlib@latest found (v1.0.1), but does not contain package github.com/bep/testmodlib/somepackage/vendor

If I add a replace directive for that module:

replace github.com/bep/testmodlib => /Users/bep/dev/go/bep/testmodlib

It compiles/runs fine:

testmodlib
somepackage
vendor package
vendors package

I understand that the vendor top level folder in a module has a special meaning, but according to https://golang.org/ref/spec#Packages that package name should be fine -- and just deleting it sounds rather harsh.

I assume this is related to #31562 -- I have not read that one in detail, but I don't see how the conclusion can be correct.

• github.com/bep/testmodlib/somepackage/vendor seem to be a valid package with a replace, and I assume that the compiler works as expected here.
• Also, a module may contain non-Go folders (test resources etc. to make the test pass).

[gopherbot]

Change https://golang.org/cl/220640 mentions this issue: cmd/go/internal/modfetch: delete unused isVendoredPackage function

[bcmills]

I assume this is related to #31562 -- I have not read that one in detail, but I don't see how the conclusion can be correct.

You are correct: this is closely related. https://play.golang.org/p/zCP7U8cKAGc

[bcmills]

CC @jayconrod @matloob, @FiloSottile FYI.

(I don't think we can plausibly fix this without invalidating checksums, though.)

[gopherbot]

Change https://golang.org/cl/220657 mentions this issue: zip: document isVendoredPackage false-positives

[FiloSottile]

I suppose we could gate this sort of changes on the go.mod version?

[jayconrod]

@FiloSottile Unfortunately not. Old versions of cmd/go should still work with go.mod files that declare newer versions. So different versions of Go would produce different sums.

[bcmills]

That proposal is #30369, but it seems unlikely to be accepted.

[dmitshur]

(I don't think we can plausibly fix this without invalidating checksums, though.)

To be more specific and confirm, do you mean without invalidating checksums of the affected modules; i.e., modules that have a non-top-level package with "/vendor" import path suffix that was unfortunately excluded from the .zip?

[jayconrod]

To be more specific and confirm, do you mean without invalidating checksums of the affected modules; i.e., modules that have a non-top-level package with "/vendor" import path suffix that was unfortunately excluded from the .zip?

Right. If we fixed this, then for a given repository at a given commit, we would include a different set of files in the module zip file. The zip file would have a different sum.

We don't have any safe mechanism for making that kind of change (though #30369, if implemented, is a possibility). The only change we can make is adding files that previously caused hard errors (for example, we could expand the set of allowed characters or increase the file size limit). We can't add files that were previously excluded.

[dmitshur]

The unfortunate label indicates this is a valid issue (though possibly fairly minor; given it affects a small percentage of possible package names) but there doesn't appear to exist any good way to make progress on resolving it.

(I don't think we can plausibly fix this without invalidating checksums, though.)

I suppose we could gate this sort of changes on the go.mod version?

Unfortunately not. Old versions of cmd/go should still work with go.mod files that declare newer versions. So different versions of Go would produce different sums.

Perhaps it can work for supported versions of Go if we gate the change on a future go.mod Go version, and teach all earlier Go releases to handle it. So by the time module versions start to declare the target go.mod Go version 1.N, the supported Go 1.(N-1) and Go 1.N releases will know to do the right thing, and Go1.(N-2) that doesn't will be unsupported. Similar to how the // +build → //go:build change was rolled out in multiple stages.

(I also considered gating on a future date, but that seems strictly worse.)

The overhead in such a change might be large enough that it'd be worth bundling multiple known module boundary improvements into one rather than doing it individually. Also, all future Go releases would need to implement both behaviours to be able to handle both old and new modules—so there are long-term costs in addition to transition costs.

[matloob]

Post go1.21 there seems to be a way to make progress on resolving this. We should consider if it's worth fixing.

cc @samthanawalla

[gopherbot]

Change https://go.dev/cl/584635 mentions this issue: x/mod: remove vendor/modules.txt from module download

[gopherbot]

Change https://go.dev/cl/619175 mentions this issue: x/mod: fix handling of vendored packages with '/vendor' in non-top-level paths