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: non-top-level packages ending in "/vendor" are omitted from module zip files #37397

Open
bep opened this issue Feb 24, 2020 · 10 comments
Labels
GoCommand cmd/go modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Unfortunate
Milestone

Comments

@bep
Copy link
Contributor

bep commented Feb 24, 2020

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
Copy link

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

@bcmills
Copy link
Contributor

bcmills commented Feb 24, 2020

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
Copy link
Contributor

bcmills commented Feb 24, 2020

CC @jayconrod @matloob, @FiloSottile FYI.

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

@gopherbot
Copy link

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

@bcmills bcmills changed the title modules: github.com/bep/testmodlib/somepackage/vendor fails cmd/go: packages named "vendor" are erroneously omitted from module zip files Feb 24, 2020
@bcmills bcmills changed the title cmd/go: packages named "vendor" are erroneously omitted from module zip files cmd/go: non-top-level packages named "vendor" are erroneously omitted from module zip files Feb 24, 2020
@bcmills bcmills changed the title cmd/go: non-top-level packages named "vendor" are erroneously omitted from module zip files cmd/go: non-top-level packages ending in "/vendor" are omitted from module zip files Feb 24, 2020
gopherbot pushed a commit that referenced this issue Feb 24, 2020
This function is apparently unused since CL 204917.

Updates #35290
Updates #37397

Change-Id: Id7f5f5d5176fdbd1c5c6227e81d0854ceafc3f12
Reviewed-on: https://go-review.googlesource.com/c/go/+/220640
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
gopherbot pushed a commit to golang/mod that referenced this issue Feb 24, 2020
I had thought that the known bug in isVendoredPackage was strictly
conservative, but it turns out not to be.

Updates golang/go#37397
Updates golang/go#31562

Change-Id: I60f6062c41ec2d116766930f751d7df083535355
Reviewed-on: https://go-review.googlesource.com/c/mod/+/220657
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@FiloSottile
Copy link
Contributor

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

@jayconrod
Copy link
Contributor

@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
Copy link
Contributor

bcmills commented Feb 25, 2020

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

@cagedmantis cagedmantis added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Feb 26, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Mar 7, 2020

(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?

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Mar 7, 2020
@jayconrod
Copy link
Contributor

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
Copy link
Contributor

dmitshur commented Jun 4, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Unfortunate
Projects
None yet
Development

No branches or pull requests

7 participants