support go modules projects #50

paivagustavo · 2019-07-02T00:54:46Z

This is the continuation of #19, I made some bad pushes and the PR was closed, so I'm opening this again.

Fixes #7

…/master

paivagustavo · 2019-07-02T01:02:35Z

@jonjohnsonjr I think I have addressed all your last requests.

About the test project that is copied to the /tmp folder I have considered using the same from cmd/ko/test but I think it would be better to simply duplicate it, as it allows this two tests projects be independent and I haven't found an elegant way to use that folder and add a go.mod file.

If you think that duplicating these files is not the way to go I can try it again.

paivagustavo · 2019-07-02T02:34:12Z

One thing to mention is that there is a lot of code duplication on the tests. I have changed it, but I think it would make this PR too overwhelming. So I'm making it to another PR as a follow-up.

jonjohnsonjr

I think it would be better to simply duplicate it, as it allows this two tests projects be independent

Sounds good to me.

I just built this and tried it on knative/serving -- it slows things down pretty tremendously... 30 seconds became 560 seconds 😿

This ends up shelling out to go list several thousand times -- I'm not sure if it's causing the scheduler to thrash or just eating up CPU, but I think it's too much of a performance regression to merge as is :/

Ideally, this is something we can fix, but if it's not we might have to make some more drastic changes to support go modules (by making it easier to detect when we should do a build, e.g. #9

cc @mattmoor

pkg/build/gobuild.go

clrprod · 2019-07-02T16:55:35Z

pkg/build/gobuild.go

@@ -86,19 +86,41 @@ func NewGo(options ...Option) (Interface, error) {
 	return gbo.Open()
 }

+// findImportPath uses the go list command to find the full import path from a package.
+func (g *gobuild) findImportPath(importPath, format string) (string, error) {


Can these calls be stored in a cache and then flushed each time a file change is detected? This will likely provide an enormous speedup.

Maybe -- we still have to call that once per unique string in the input yaml, so this would help a bit but not that much.

Hmmm - could calling go list once at the top level work? That way you could avoid having to deal with lots of go list calls.

We could do this in batch, go list accepts multiples packages as argument, would need to try this out. @jonjohnsonjr can you share to me how to reproduce the ko resolve from knative that you tried?

I'm also trying to figure someway to prune some of these unique strings on the input yaml, because not every string are actually valid packages names right?

can you share to me how to reproduce the ko resolve from knative that you tried?

Clone https://github.com/knative/serving and run ko resolve -f config/.

not every string are actually valid packages names right

Yeah we could do some filtering, but I think most strings are going to be potentially valid packages.

I think I got it to a time that is fine. The biggest culprit for this slow down was that the method isSupportedReference was being called a lot of times.

I have done two things:

Pruned some invalid package names (found a way based on the internal load packing from the Go command);

Cached the result of this method for each import path;

We sure could try to batch go list calls, but that would make it require a lot more effort and I think that should be done in the future.

Also I have some doubts of which strings we do try to build and replace on the YAML, currently we try check every string from the YAML, such as: normal strings, map keys, map values and array, do we actually need them all?

@jonjohnsonjr I tested out on the knative's serving repo and it was down to 35~40 seconds on my machine, can you run this test on your side as well?

Friendly ping @jonjohnsonjr.
Also @clrprod as you pointed out to cache the results.

jonjohnsonjr · 2019-07-08T21:04:58Z

pkg/build/shared.go

@@ -65,15 +69,42 @@ func (c *Caching) Build(ip string) (v1.Image, error) {
 	return f.Get()
 }

+// SafeArg is used to check if the import path is valid.
+// Copy of github.com/golang/go/blob/master/src/cmd/go/internal/load/pkg.go
+func SafeArg(name string) bool {


I'd move this into gobuild.go

jonjohnsonjr · 2019-07-08T22:55:25Z

Going to think out loud a bit here, let me know if I'm misunderstanding something about go modules or have missed something:

In a world with just GOPATH, there was a single namespace for packages. They were all rooted at GOPATH, and you could simply read from the filesystem to find the package.

With go modules, there is a second namespace introduced by the go.mod file. The "module" directive gives a root for the current directory that doesn't require you to be on GOPATH. Any packages in that directory (or subdirecties) should be prefixed by the module path.

So here's what I'm thinking:

We call go list -mod=readonly -m once. If successful, this should print the current module path. We can look for this prefix on any importpaths to determine if we should try to build the package. We might need to still call out to go list to determine if it's a main package (or something), but it should be much faster than calling go list on everything.

If go list -mod=readonly -m fails, we should just do what we currently do, which is look for the package on GOPATH.

Some things I'm not sure about:

Should we still fall back to looking in GOPATH for packages if a project is using go modules? Or do we only expect to build/reference packages that are within a single project?

Is it okay that we only look in the current directory for go.mod files? I believe that's how modules are expected to work, but I'm not sure how people do mono-repos with go modules.

xmlking · 2019-07-09T00:52:01Z

@jonjohnsonjr as for I know, in go-module enabled project (which is also located outside GOPATH) , we still run go build -o bin/account-srv ./srv/account in project root(where go.mod is located) to build binary. no additional setting needed.

I think the build process is same for regular projects located in GOPATH as well.

It is fair to expect only to build/reference packages that are within a single project/monorepo, anyway if importpaths are not part of current project, we can still use fill path to build.

The flow you stated above should work.
If it is go-module enabled project, and importpaths prefixed with namespace from go list -mod=readonly -m , then subtract the namespace and then run go build ..., otherwise use full importpaths as source path.

e.g.,

# go-module enabled projects  (truncation prefix from  importpaths)
go build -o bin/account-srv ./srv/account
go build -o bin/emailer-srv ./srv/emailer

# non  go-module projects (no truncation for importpaths)
go build -o bin/account-srv github.com/xmlking/micro-starter-kit/srv/account
go build -o bin/emailer-srv github.com/xmlking/micro-starter-kit/srv/emailer

you can use my repo for testing as it is monorepo and using go-module

paivagustavo added 4 commits June 22, 2019 18:37

first draft to support module support.

e935949

renaming method and paremeters and code cleaning

6352a8d

Merge branch 'master' of https://github.com/paivagustavo/ko into fork…

9d12c73

…/master

refactoring tests

41985e8

add eol to test file

f9c83a1

paivagustavo mentioned this pull request Jul 2, 2019

Support for go modules #19

Closed

jonjohnsonjr reviewed Jul 2, 2019

View reviewed changes

n3wscott reviewed Jul 2, 2019

View reviewed changes

pkg/build/gobuild.go Show resolved Hide resolved

clrprod reviewed Jul 2, 2019

View reviewed changes

caching isSupportedReference

67c11d2

jonjohnsonjr reviewed Jul 8, 2019

View reviewed changes

jonjohnsonjr mentioned this pull request Jul 8, 2019

ko with monorepo #51

Closed

imjasonh mentioned this pull request Jul 10, 2019

Implement "strict mode" which requires ko:// prefix for import paths #58

Merged

jonjohnsonjr mentioned this pull request Jul 12, 2019

Add go module support #60

Merged

paivagustavo closed this Jul 13, 2019

chizhg mentioned this pull request Apr 1, 2020

Migrate all Knative projects to use Go Modules knative/test-infra#1875

Closed

jonjohnsonjr mentioned this pull request Feb 3, 2021

Refactor go builder to stop using go/build #305

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support go modules projects #50

support go modules projects #50

paivagustavo commented Jul 2, 2019

paivagustavo commented Jul 2, 2019

paivagustavo commented Jul 2, 2019

jonjohnsonjr left a comment •

edited

clrprod Jul 2, 2019

jonjohnsonjr Jul 2, 2019

clrprod Jul 2, 2019

paivagustavo Jul 3, 2019

paivagustavo Jul 3, 2019

jonjohnsonjr Jul 3, 2019

paivagustavo Jul 5, 2019

paivagustavo Jul 5, 2019

paivagustavo Jul 8, 2019

jonjohnsonjr Jul 8, 2019

jonjohnsonjr commented Jul 8, 2019

xmlking commented Jul 9, 2019 •

edited

support go modules projects #50

support go modules projects #50

Conversation

paivagustavo commented Jul 2, 2019

paivagustavo commented Jul 2, 2019

paivagustavo commented Jul 2, 2019

jonjohnsonjr left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonjohnsonjr commented Jul 8, 2019

xmlking commented Jul 9, 2019 • edited

jonjohnsonjr left a comment •

edited

xmlking commented Jul 9, 2019 •

edited