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.

Sign up for GitHub

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

support go modules projects #50

Closed
wants to merge 6 commits into from
Closed

support go modules projects #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e935949
first draft to support module support.
paivagustavo Mar 26, 2019
6352a8d
renaming method and paremeters and code cleaning
paivagustavo Jun 22, 2019
9d12c73
Merge branch 'master' of https://github.com/paivagustavo/ko into fork…
paivagustavo Jul 1, 2019
41985e8
refactoring tests
paivagustavo Jul 2, 2019
f9c83a1
add eol to test file
paivagustavo Jul 2, 2019
67c11d2
caching isSupportedReference
paivagustavo Jul 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 44 additions & 21 deletions pkg/build/gobuild.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ import (
"bytes"
"compress/gzip"
"errors"
gb "go/build"
"io"
"io/ioutil"
"log"
"os"
"os/exec"
"path/filepath"
"strings"

v1 "github.com/google/go-containerregistry/pkg/v1"
"github.com/google/go-containerregistry/pkg/v1/mutate"
Expand Down Expand Up @@ -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) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cmd := exec.Command("go", "list", "-f", "{{."+format+"}}", importPath)
stdout, err := cmd.Output()
if err != nil {
return "", err
}

outputs := strings.Split(string(stdout), "\n")
if len(outputs) == 0 {
return "", errors.New("could not find specified import path: " + importPath)
}

output := outputs[0]
if output == "" {
return "", errors.New("import path is ambiguous: " + importPath)
}

return output, nil
}

// IsSupportedReference implements build.Interface
//
// Only valid importpaths that provide commands (i.e., are "package main") are
// supported.
func (*gobuild) IsSupportedReference(s string) bool {
p, err := gb.Import(s, gb.Default.GOPATH, gb.ImportComment)
func (g *gobuild) IsSupportedReference(s string) bool {
output, err := g.findImportPath(s, "Name")
paivagustavo marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return false
}
return p.IsCommand()

return output == "main"
}

func build(ip string, disableOptimizations bool) (string, error) {
func build(importPath string, disableOptimizations bool) (string, error) {
tmpDir, err := ioutil.TempDir("", "ko")
if err != nil {
return "", err
Expand All @@ -112,7 +134,7 @@ func build(ip string, disableOptimizations bool) (string, error) {
args = append(args, "-gcflags", "all=-N -l")
}
args = append(args, "-o", file)
args = append(args, ip)
args = append(args, importPath)
cmd := exec.Command("go", args...)

// Last one wins
Expand All @@ -123,7 +145,7 @@ func build(ip string, disableOptimizations bool) (string, error) {
cmd.Stderr = &output
cmd.Stdout = &output

log.Printf("Building %s", ip)
log.Printf("Building %s", importPath)
if err := cmd.Run(); err != nil {
os.RemoveAll(tmpDir)
log.Printf("Unexpected error running \"go build\": %v\n%v", err, output.String())
Expand Down Expand Up @@ -216,18 +238,19 @@ func tarBinary(name, binary string) (*bytes.Buffer, error) {
return buf, nil
}

func kodataPath(s string) (string, error) {
p, err := gb.Import(s, gb.Default.GOPATH, gb.ImportComment)
// kodataPath returns the absolute path for the kodata path for a given package
func (g *gobuild) kodataPath(importPath string) (string, error) {
absolutePath, err := g.findImportPath(importPath, "Dir")
if err != nil {
return "", err
}
return filepath.Join(p.Dir, "kodata"), nil
return filepath.Join(absolutePath, "kodata"), nil
}

// Where kodata lives in the image.
const kodataRoot = "/var/run/ko"

func tarKoData(importpath string) (*bytes.Buffer, error) {
func (g *gobuild) tarKoData(importPath string) (*bytes.Buffer, error) {
buf := bytes.NewBuffer(nil)
// Compress this before calling tarball.LayerFromOpener, since it eagerly
// calculates digests and diffids. This prevents us from double compressing
Expand All @@ -239,7 +262,7 @@ func tarKoData(importpath string) (*bytes.Buffer, error) {
tw := tar.NewWriter(gw)
defer tw.Close()

root, err := kodataPath(importpath)
root, err := g.kodataPath(importPath)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -297,17 +320,17 @@ func tarKoData(importpath string) (*bytes.Buffer, error) {
}

// Build implements build.Interface
func (gb *gobuild) Build(s string) (v1.Image, error) {
func (g *gobuild) Build(importPath string) (v1.Image, error) {
// Do the build into a temporary file.
file, err := gb.build(s, gb.disableOptimizations)
file, err := g.build(importPath, g.disableOptimizations)
if err != nil {
return nil, err
}
defer os.RemoveAll(filepath.Dir(file))

var layers []mutate.Addendum
// Create a layer from the kodata directory under this import path.
dataLayerBuf, err := tarKoData(s)
dataLayerBuf, err := g.tarKoData(importPath)
if err != nil {
return nil, err
}
Expand All @@ -322,12 +345,12 @@ func (gb *gobuild) Build(s string) (v1.Image, error) {
Layer: dataLayer,
History: v1.History{
Author: "ko",
CreatedBy: "ko publish " + s,
CreatedBy: "ko publish " + importPath,
Comment: "kodata contents, at $KO_DATA_PATH",
},
})

appPath := filepath.Join(appDir, appFilename(s))
appPath := filepath.Join(appDir, appFilename(importPath))

// Construct a tarball with the binary and produce a layer.
binaryLayerBuf, err := tarBinary(appPath, file)
Expand All @@ -345,13 +368,13 @@ func (gb *gobuild) Build(s string) (v1.Image, error) {
Layer: binaryLayer,
History: v1.History{
Author: "ko",
CreatedBy: "ko publish " + s,
CreatedBy: "ko publish " + importPath,
Comment: "go build output, at " + appPath,
},
})

// Determine the appropriate base image for this import path.
base, err := gb.getBase(s)
base, err := g.getBase(importPath)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -381,8 +404,8 @@ func (gb *gobuild) Build(s string) (v1.Image, error) {
}

empty := v1.Time{}
if gb.creationTime != empty {
return mutate.CreatedAt(image, gb.creationTime)
if g.creationTime != empty {
return mutate.CreatedAt(image, g.creationTime)
}
return image, nil
}
Loading