Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

makelib: introduce --enable-incremental-build, enabling "go install" #3553

Merged
merged 1 commit into from
Jan 30, 2017

Conversation

s-urbaniak
Copy link
Contributor

@s-urbaniak s-urbaniak commented Jan 20, 2017

This change decreases incremental compilation time by 50% on my machine
for stage0 builds.

stage1 binaries are much smaller so there is no significant improvement
in compilation speed.

Small change in rkt/main.go:

$ time make
  GO           github.com/coreos/rkt/rkt
  ACTOOL       build-rkt/target/bin/stage1-coreos.aci

real    0m13.086s
user    0m11.797s
sys 0m1.323s

Small change in stage1/init/init.go:

$ time make
  GO           github.com/coreos/rkt/stage1/init
  ACTOOL       build-rkt/target/bin/stage1-coreos.aci

real    0m11.615s
user    0m10.047s
sys 0m1.083s

The same with "go build":

Small change in rkt/main.go:

$ time make
  GO           github.com/coreos/rkt/rkt
  ACTOOL       build-rkt/target/bin/stage1-coreos.aci

real    0m21.573s
user    0m32.907s
sys 0m2.177s

Small change in stage1/init/init.go:

$ time make
  GO           github.com/coreos/rkt/stage1/init
  ACTOOL       build-rkt/target/bin/stage1-coreos.aci

real    0m11.365s
user    0m10.777s
sys 0m1.083s

@lucab
Copy link
Member

lucab commented Jan 21, 2017

It looks like most of the workers are unhappy about the cleaning step now.

@s-urbaniak
Copy link
Contributor Author

ack, the quickrm tool needs some massaging, since artifacts in gopath/bin and gopath/pkg are out of our control, hence we have to os.RemoveAll() for directories.

@s-urbaniak
Copy link
Contributor Author

we might also have some issue with fedora 23, since go 1.5.3 might not yet support the -pkgdir option for non-cgo builds.

let's see what the CI is saying in the next round.

@s-urbaniak
Copy link
Contributor Author

oh, interesting, the arm64 builders also seem to put the artifacts in a different dir.

@s-urbaniak s-urbaniak changed the title makelib: use "go install" in favor of "go build" [WIP] makelib: use "go install" in favor of "go build" Jan 21, 2017
@s-urbaniak
Copy link
Contributor Author

nice, only arm64 needs a fix

@s-urbaniak
Copy link
Contributor Author

arm64 places its binaries under $GOPATH/bin/linux_arm64, i.e. /home/travis/gopath/src/github.com/coreos/rkt/build-rkt-1.23.0+git/gopath/bin/linux_arm64/rkt

@s-urbaniak s-urbaniak force-pushed the fastbuild branch 4 times, most recently from 82ed97b to 09efcb5 Compare January 22, 2017 12:36
@s-urbaniak
Copy link
Contributor Author

The fedora-24 failure is a known flake.

The strange thing is that this still fails on jenkins with the cross-compilation build, erroring with:

aarch64-linux-gnu-gcc: error: unrecognized command line option '-m64'

Indeed gcc-arm doesn't support -m64 according to https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html.

I guess this is an option set in a x86 build for some other binary and seems to intertwine here. I'll try to reproduce this locally.

@squeed
Copy link
Contributor

squeed commented Jan 23, 2017

AMAZING. I appreciate this beyond belief.

@s-urbaniak
Copy link
Contributor Author

yeah, i just need to fix arm64, I think I have some silliness there left.

@s-urbaniak s-urbaniak force-pushed the fastbuild branch 4 times, most recently from b07f8a0 to b72887e Compare January 24, 2017 19:41
@s-urbaniak s-urbaniak changed the title [WIP] makelib: use "go install" in favor of "go build" makelib: use "go install" in favor of "go build" Jan 24, 2017
@s-urbaniak
Copy link
Contributor Author

I gated this behind a ./configure --enable-incremental-build, as cross-compilation was hard (impossible?) to get right and final builds are recommended using go build anyways.

Our build system actually builds also native binaries, even when in a cross compilation scenario, i.e. depsgen is built natively amongst others. This complicates things so I suggest to keep this as least intrusive as possible.

@s-urbaniak s-urbaniak changed the title makelib: use "go install" in favor of "go build" makelib: introduce --enable-incremental-build, enabling "go install" Jan 24, 2017
@s-urbaniak s-urbaniak requested review from lucab and squeed January 24, 2017 19:50
@@ -161,5 +161,14 @@ This option to enable [logging to the TPM][rkt-tpm] is set by default. For loggi

This option to allow building rkt with go having known security issues is unset by default. Use it with caution.

## Development

#### `--enable-incremental-build`
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want to add a mention that you should not build releases with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

@squeed
Copy link
Contributor

squeed commented Jan 26, 2017

A minor documentation suggestion, but LGTM.

This change decreases incremental compilation time by 50% on my machine
for stage0 builds.

stage1 binaries are much smaller so there is no significant improvement
in compilation speed.

Small change in rkt/main.go:
```
$ time make
  GO           github.com/coreos/rkt/rkt
  ACTOOL       build-rkt/target/bin/stage1-coreos.aci

real    0m13.086s
user    0m11.797s
sys 0m1.323s
```

Small change in stage1/init/init.go:
```
$ time make
  GO           github.com/coreos/rkt/stage1/init
  ACTOOL       build-rkt/target/bin/stage1-coreos.aci

real    0m11.615s
user    0m10.047s
sys 0m1.083s
```

The same with "go build":

Small change in rkt/main.go:
```
$ time make
  GO           github.com/coreos/rkt/rkt
  ACTOOL       build-rkt/target/bin/stage1-coreos.aci

real    0m21.573s
user    0m32.907s
sys 0m2.177s
```

Small change in stage1/init/init.go:
```
$ time make
  GO           github.com/coreos/rkt/stage1/init
  ACTOOL       build-rkt/target/bin/stage1-coreos.aci

real    0m11.365s
user    0m10.777s
sys 0m1.083s
```
@lucab lucab merged commit b80b888 into rkt:master Jan 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants