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

Closed
bep opened this issue Feb 24, 2020 · 13 comments
Closed
Assignees
Labels
GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done. 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
Contributor

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
Contributor

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.

@matloob
Copy link
Contributor

matloob commented Jun 18, 2024

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

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

@samthanawalla samthanawalla modified the milestones: Unplanned, Go1.24 Oct 9, 2024
@samthanawalla samthanawalla self-assigned this Oct 9, 2024
gopherbot pushed a commit to golang/mod that referenced this issue Oct 9, 2024
This fixes a bug where vendor/modules.txt was accidently included during
a module download.
This CL trims this file for 1.24 modules and beyond. We cannot remove this
for earlier Go versions because this would alter checksums and cause a
checksum failure.
This CL also adds the ability to case on the Go version in the root's
go.mod file, enabling future behavior changes if necessary.

Fixes: golang/go#63395
Updates: golang/go#37397
Change-Id: I4a4f2174b0f5e79c7e5c516e0db3c91e7d2ae4d9
Reviewed-on: https://go-review.googlesource.com/c/mod/+/584635
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
@gopherbot
Copy link
Contributor

gopherbot commented Oct 9, 2024

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

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done. Unfortunate
Projects
None yet
Development

No branches or pull requests

9 participants