vendor paths are broken with go modules #152
Comments
|
Not that for Serving, I have been poking at this in the context of: knative/serving#7734, which as of now has not yet landed. |
|
Tekton should be building from the I was able to build the sample Knative image successfully: Out of curiosity, what versions of go/ko are you using, and does it work for you if you run |
|
I tried that: I am building The problem may not just be my version of Go, but also the version of Go that |
|
Oh... You were on master, which still works. Yeah the go mod change hasn't landed (though it should soon). I was hoping you pulled my PR in 😬 Updating to 1.14 doesn't fix anything for me. I will add back some instrumentation to confirm my statement above about Tekton and the |
|
Strangely I think the relative path problems with |
This change more or less completely changes how `ko://` is handled internally to `ko`, but the user-facing changes should only be net-positive. `ko://` was previously stripped at the highest level, and the build logic was unaware, which had some undesirable diagnostic/functional implications that are collectively addressed in this change. With this change, the `ko://` prefix is preserved and passed to the build logic, which internally parses a new `reference` type (this was useful to have Go's type checker find all of the places that needed fixing). The main functional differences are: 1. If a reference is prefixed with `ko://` we will now fail fast in `IsSupportedReference` regardless of whether `--strict` is passed. 2. If a reference is prefixed with `ko://` it will bypass the prefix check, which allows the use of `ko://github.com/another/repo` that references a vendored binary package. For `2.` the absence of the module prefix causes the filtering logic Jon introduced to avoid the reference. This was critical for efficiency when `ko://` isn't around because we feed every string in the yaml through it, but when the user has explicitly decorated things it's the perfect thing to be sensitive to. Fixes: ko-build#146 Fixes: ko-build#152
This change more or less completely changes how `ko://` is handled internally to `ko`, but the user-facing changes should only be net-positive. `ko://` was previously stripped at the highest level, and the build logic was unaware, which had some undesirable diagnostic/functional implications that are collectively addressed in this change. With this change, the `ko://` prefix is preserved and passed to the build logic, which internally parses a new `reference` type (this was useful to have Go's type checker find all of the places that needed fixing). The main functional differences are: 1. If a reference is prefixed with `ko://` we will now fail fast in `IsSupportedReference` regardless of whether `--strict` is passed. 2. If a reference is prefixed with `ko://` it will bypass the prefix check, which allows the use of `ko://github.com/another/repo` that references a vendored binary package. For `2.` the absence of the module prefix causes the filtering logic Jon introduced to avoid the reference. This was critical for efficiency when `ko://` isn't around because we feed every string in the yaml through it, but when the user has explicitly decorated things it's the perfect thing to be sensitive to. Fixes: #146 Fixes: #152
We've been talking about this a bit in Knative slack in #ko, but I wanted to track this here in an issue, which is more durable and searchable.
The gist is that there are a number of projects that vendor a binary and then embed the following in yaml to publish the vendored binary:
github.com/foo/bar/vendor/github.com/baz/blahI was under the mistaken impression that this worked fine because Tekton uses this for the
gcs-fetcherimage, but it turns out that isn't actually building from the local vendor tree, but the "latest" release of Tekton. We caught this on the Knative side because we're still tagging our releases as "pre-release" so Github's "latest" release voodoo doesn't mask the issue for us like it did for Tekton.For Tekton, the following works for the same reason:
However, a similar command in Knative Serving fails (due to the "no latest release" problem):
Interestingly, when I adapt these both to relative paths, they BOTH fail, but for a different reason!
In this case they fail because
qualifyLocalImport(due topackages.Load) returns the unqualified import path (e.g.github.com/baz/blah) , but prior to go modules it returned the fully-qualified path (e.g.github.com/foo/bar/vendor/github.com/baz/blah).The nearest workaround I've been able to find thus far is to drop the condition around this call:
https://github.com/google/ko/blob/f45bc13ded83af4cfa337910d4ba892015f5d1dc/pkg/build/gobuild.go#L142-L150
... which would let us reference
ko://github.com/baz/blahfrom withingit.luolix.top/foo/barand (I believe) have it respect the version of that dependency fromgo.mod🤞The main thing that gives me pause here is that the PR (#60) that introduced go mod support referenced this as a pretty important performance optimization 😬
I'm hoping that either @imjasonh or @jonjohnsonjr has some insights here.
The text was updated successfully, but these errors were encountered: