Build improvements #443

smoser · 2023-03-14T16:25:25Z

workflow:

add Setup Environment section that defines GOPATH and set GOPATH to /.build/gopath and GOCACHE to /.build/gocache/gocache.

This means that all 'go' operations in subsequent sections will have that variable set. The go toolchain will then correctly re-use anything it can throughout the build.

Note these are the same values that use of Make will set both inside and outside the stacker build.

(stacker build dir is bind'd into the build environment and Make will set the variable correctly inside also, so the static build will benefit from a cache that the dynamic build did)
Separate out 'go mod download' from the build. This makes better information available on how long operations things took.

Because GOMODPATH is shared, these will used both by the stacker-dynamic and stacker binary builds.

Makefile:

Allow caller to use a already-built "level 1 stacker" to build the current source. By setting LEVEL1_STACKER as below, you can avoid building (and re-building) the stacker that is used to build the static stacker.

make LEVEL1_STACKER=/usr/local/bin/stacker
export GOPATH and GOCACHE (above)
change GO_SRC to only look under pkg/ and cmd/ directories for go source files. Previously it would search the .build directory which is at best a waste of time and often produced errors to stderr about not being able to read directories.
change stacker's dependency from cmd/stacker/lxc-wrapper/lxc-wrapper to cmd/stacker/lxc-wrapper/lxc-wrapper.c . stacker is built inside the stacker environment and the build.yaml file removes the existing lxc-wrapper, so the internal build will never use lxc-wrapper from the external environment.

At some point we could use the caching in github actions to re-use a .build/gopath directory to get caching across actions.

What type of PR is this?

Which issue does this PR fix:

What does this PR do / Why do we need it:

If an issue # is not available please add repro steps and logs showing the issue:

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?

Does this PR introduce any user-facing change?:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

rchincha · 2023-03-14T19:47:14Z

Makefile

@@ -17,25 +22,34 @@ STACKER_BUILD_UBUNTU_IMAGE?=$(STACKER_DOCKER_BASE)ubuntu:latest
 LXC_CLONE_URL?=https://github.com/lxc/lxc
 LXC_BRANCH?=stable-5.0

-stacker: stacker-dynamic
-	./stacker-dynamic --debug $(STACKER_OPTS) build \
+LEVEL1_STACKER ?= ./stacker-dynamic


maybe call this BOOTSTRAP_STACKER

I don't love the name LEVEL1_STACKER either, but BOOTSTRAP_STACKER felt like it would be a boolean, ie: Should I bootstrap?

STACKER_BOOTSTRAP

huh. i +1'd to the STAGE1_STACKER and then it changed to STACKER_BOOTSTRAP. i like the STAGE1_STACKER, just pushed that.

.github/workflows/build.yaml

smoser · 2023-03-15T15:28:43Z

I looked a bit deeper into the failure that c-i is showing. It reproduces locally with a simple 'make test' from this branch.
Here is what I found:

go test here is run with -coverpkg ./... and ./... as the package to test.
To reproduce, GOPATH must be set to a directory under the directory where 'go test' is run then this will reproduce.
-coverage=atomic is required to make it fail (cmd/cover: cover mode "atomic" generates invalid code for files without a newline at EOF golang/go#58370) .
I think 'go test' with packages specified as './...' is just confused when the packages it is reading in GOPATH are found under . and thus incorrectly get included in './...'
subsequent runs get worse if the GOPATH directory does not begin with a '.', as if the first run didn't include some of the packages that were downloaded. Example, GOPATH=mypath gives the same results as GOPATH=.mypath on the first run. On the second run, GOPATH=mypath is worse, complaining about " no required module provides package github.com/bsphere/le_go..." for many packages.
it is odd to me but true that if .build/gopath is populated from a previous run there is no problem with those files unless GOPATH is set. So running this:
- this will fail, but populate .my-gopath
  
  GOPATH=$PWD/.my-gopath go test -v -trimpath -cover -coverpkg ./... -coverprofile=coverage.txt -covermode=atomic ./...
- this will not fail even after .my-gopath is populated.
  
  go test -v -trimpath -cover -coverpkg ./... -coverprofile=coverage.txt -covermode=atomic ./...`

So the solutions we have

Move GOPATH out from under the build tree (just set BUILD_D to some other
location)
change how 'go test' is invoked, to not use './...' from the top level.
Instead, we could do (cd pkg && go test ./...)

For reference, the error from c-i is:

make check VERSION_FULL=3bd3bdd8943b043bb092ba4847591732e3adc007 PRIVILEGE_LEVEL=unpriv
  shell: /usr/bin/bash -e {0}
  env:
    SHA: 3bd3bdd
    GOPATH: /home/runner/work/stacker/stacker/.build/gopath
    GOCACHE: /home/runner/work/stacker/stacker/.build/gopath/gocache
    PATH: /home/runner/work/stacker/stacker/.build/gopath/bin:/home/runner/go/bin:/opt/hostedtoolcache/go/1.20.1/x64/bin:/home/runner/.local/bin:/opt/pipx_bin:/home/runner/.cargo/bin:/home/runner/.config/composer/vendor/bin:/usr/local/.ghcup/bin:/home/runner/.dotnet/tools:/snap/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin
    REGISTRY_URL: localhost:5000
go build -tags "exclude_graphdriver_btrfs exclude_graphdriver_devicemapper containers_image_openpgp osusergo netgo " -ldflags "-X main.version=3bd3bdd8943b043bb092ba4847591732e3adc007 -X main.lxc_version=5.0.0~git2209-g5a7b9ce67 " -o stacker-dynamic ./cmd/stacker
...
go mod tidy
go: downloading github.com/tj/assert v0.0.3
go: downloading github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826
go: downloading gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c
go: downloading github.com/xeipuuv/gojsonschema v1.2.0
go: downloading github.com/google/go-cmp v0.5.9
go: downloading github.com/kr/pretty v0.2.1
go: downloading gotest.tools/v3 v3.0.3
go: downloading gotest.tools v2.2.0+incompatible
go: downloading github.com/imdario/mergo v0.3.13
go: downloading github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415
go: downloading github.com/kr/text v0.2.0
go: downloading github.com/go-test/deep v1.1.0
go: downloading github.com/xeipuuv/gojsonpointer v0.0.0-20190905194746-02993c407bfb
go: downloading github.com/moby/term v0.0.0-20210619224110-3f7ff695adc6
go: downloading github.com/morikuni/aec v1.0.0
go: downloading github.com/tidwall/pretty v1.2.0
go: downloading github.com/prometheus/client_golang v1.14.0
go: downloading github.com/prometheus/client_model v0.3.0
go: downloading github.com/honeycombio/beeline-go v1.10.0
go: downloading github.com/honeycombio/libhoney-go v1.16.0
go: downloading github.com/jmhodges/clock v0.0.0-20160418191101-880ee4c33548
go: downloading golang.org/x/time v0.2.0
go: downloading github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1
go: downloading github.com/beorn7/perks v1.0.1
go: downloading github.com/prometheus/procfs v0.9.0
go: downloading gopkg.in/alexcesaro/statsd.v2 v2.0.0
go: downloading github.com/facebookgo/muster v0.0.0-20150708232844-fd3d7953fd52
go: downloading github.com/vmihailenco/msgpack/v5 v5.3.5
go: downloading github.com/matttproud/golang_protobuf_extensions v1.0.4
go: downloading github.com/facebookgo/clock v0.0.0-20150410010913-600d898af40a
go: downloading github.com/facebookgo/limitgroup v0.0.0-20150612190941-6abd8d71ec01
go: downloading github.com/vmihailenco/tagparser/v2 v2.0.0
go fmt ./... && ([ -z true ] || git diff --exit-code)
bash test/static-analysis.sh
go test -v -trimpath -cover -coverpkg ./... -coverprofile=coverage.txt -covermode=atomic -tags "exclude_graphdriver_btrfs exclude_graphdriver_devicemapper containers_image_openpgp osusergo netgo" ./...
# github.com/modern-go/concurrent
Error: .build/gopath/pkg/mod/github.com/modern-go/concurrent@v0.0.0-20180306012644-bacd9c7ef1dd/log.go:13:48: syntax error: unexpected var after top level declaration
# github.com/modern-go/reflect2
Error: .build/gopath/pkg/mod/github.com/modern-go/reflect2@v1.0.2/go_above_118.go:23:2: syntax error: unexpected var after top level declaration
# github.com/cyberphone/json-canonicalization/go/src/webpki.org/jsoncanonicalizer
Error: .build/gopath/pkg/mod/github.com/cyberphone/json-canonicalization@v0.0.0-20220623050100-57a0ce2678a7/go/src/webpki.org/jsoncanonicalizer/jsoncanonicalizer.go:378:2: syntax error: unexpected var after top level declaration
?   	stackerbuild.io/stacker/pkg/container	[no test files]
?   	stackerbuild.io/stacker/pkg/container/idmap	[no test files]
?   	stackerbuild.io/stacker/pkg/embed-exec	[no test files]
=== RUN   TestAllowMissingVerityData
...
FAIL	stackerbuild.io/stacker/pkg/stacker [build failed]
..
FAIL
make: *** [Makefile:58: lint] Error 1
Error: Process completed with exit code 2.

smoser · 2023-03-15T15:29:35Z

I think it makes sense to just change the default 'BUILD_D' (and thus GOPATH which is derived from it) to be outside of the source directory.

workflow: * add Setup Environment section that defines GOPATH and set GOPATH to <checkoutdir>/.build/gopath and GOCACHE to <checkoutdir>/.build/gocache/gocache. This means that all 'go' operations in subsequent sections will have that variable set. The go toolchain will then correctly re-use anything it can throughout the build. Note these are the same values that use of Make will set both inside and outside the stacker build. (stacker build dir is bind'd into the build environment and Make will set the variable correctly inside also, so the static build will benefit from a cache that the dynamic build did) Note comment in PR project-stacker#443 indicating issues with 'go test ./...' if GOPATH is under the current working directory. * Separate out 'go mod download' from the build. This makes better information available on how long operations things took. Because GOMODPATH is shared, these will used both by the stacker-dynamic and stacker binary builds. Makefile: * Allow caller to use a already-built "level 1 stacker" to build the current source. By setting LEVEL1_STACKER as below, you can avoid building (and re-building) the stacker that is used to build the static stacker. make LEVEL1_STACKER=/usr/local/bin/stacker * export GOPATH and GOCACHE (above) * change GO_SRC to only look under pkg/ and cmd/ directories for go source files. Previously it would search the .build directory which is at best a waste of time and often produced errors to stderr about not being able to read directories. * change stacker's dependency from cmd/stacker/lxc-wrapper/lxc-wrapper to cmd/stacker/lxc-wrapper/lxc-wrapper.c . stacker is built inside the stacker environment and the build.yaml file removes the existing lxc-wrapper, so the internal build will never use lxc-wrapper from the external environment. At some point we could use the caching in github actions to re-use a .build/gopath directory to get caching across actions. Signed-off-by: Scott Moser <smoser@brickies.net>

workflow: * add Setup Environment section that defines GOPATH and set GOPATH to <checkoutdir>/../stacker-build/gopath and GOCACHE to <checkoutdir>-build/gopath/gocache. This means that all 'go' operations in subsequent sections will have that variable set. The go toolchain will then correctly re-use anything it can throughout the build. Note these are the same values that use of Make will set both inside and outside the stacker build. (stacker build dir is bind'd into the build environment and Make will set the variable correctly inside also, so the static build will benefit from a cache that the dynamic build did) Note comment in PR project-stacker#443 indicating issues with 'go test ./...' if GOPATH is under the current working directory. * Separate out 'go mod download' from the build. This makes better information available on how long operations things took. Because GOMODPATH is shared, these will used both by the stacker-dynamic and stacker binary builds. Makefile: * Allow caller to use a already-built "level 1 stacker" to build the current source. By setting LEVEL1_STACKER as below, you can avoid building (and re-building) the stacker that is used to build the static stacker. make LEVEL1_STACKER=/usr/local/bin/stacker * export GOPATH and GOCACHE (above) * change GO_SRC to only look under pkg/ and cmd/ directories for go source files. Previously it would search the .build directory which is at best a waste of time and often produced errors to stderr about not being able to read directories. * change stacker's dependency from cmd/stacker/lxc-wrapper/lxc-wrapper to cmd/stacker/lxc-wrapper/lxc-wrapper.c . stacker is built inside the stacker environment and the build.yaml file removes the existing lxc-wrapper, so the internal build will never use lxc-wrapper from the external environment. At some point we could use the caching in github actions to re-use a .build/gopath directory to get caching across actions. Signed-off-by: Scott Moser <smoser@brickies.net>

workflow: * add Setup Environment section that defines GOPATH and set GOPATH to <checkoutdir>/../stacker-build/gopath and GOCACHE to <checkoutdir>-build/gopath/gocache. This means that all 'go' operations in subsequent sections will have that variable set. The go toolchain will then correctly re-use anything it can throughout the build. Note these are the same values that use of Make will set both inside and outside the stacker build. (stacker build dir is bind'd into the build environment and Make will set the variable correctly inside also, so the static build will benefit from a cache that the dynamic build did) Note comment in PR project-stacker#443 indicating issues with 'go test ./...' if GOPATH is under the current working directory. * Separate out 'go mod download' from the build. This makes better information available on how long operations things took. Because GOMODPATH is shared, these will used both by the stacker-dynamic and stacker binary builds. Makefile: * Allow caller to use a already-built "level 1 stacker" to build the current source. By setting LEVEL1_STACKER as below, you can avoid building (and re-building) the stacker that is used to build the static stacker. make LEVEL1_STACKER=/usr/local/bin/stacker * change 'go test' invocation to explicitly list the stacker package rather than using ./... which can cause problems if GOPATH is under the current working dir. * export GOPATH and GOCACHE (above) * change GO_SRC to only look under pkg/ and cmd/ directories for go source files. Previously it would search the .build directory which is at best a waste of time and often produced errors to stderr about not being able to read directories. * change stacker's dependency from cmd/stacker/lxc-wrapper/lxc-wrapper to cmd/stacker/lxc-wrapper/lxc-wrapper.c . stacker is built inside the stacker environment and the build.yaml file removes the existing lxc-wrapper, so the internal build will never use lxc-wrapper from the external environment. At some point we could use the caching in github actions to re-use a .build/gopath directory to get caching across actions. Signed-off-by: Scott Moser <smoser@brickies.net>

codecov · 2023-03-15T16:12:49Z

Codecov Report

Merging #443 (ece2499) into main (8e267fc) will not change coverage.
The diff coverage is n/a.

❗ Current head ece2499 differs from pull request most recent head 474bf1a. Consider uploading reports for the commit 474bf1a to get more accurate results

@@           Coverage Diff           @@
##             main     #443   +/-   ##
=======================================
  Coverage   11.87%   11.87%           
=======================================
  Files          45       45           
  Lines        5551     5551           
=======================================
  Hits          659      659           
  Misses       4775     4775           
  Partials      117      117

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

smoser · 2023-03-15T16:15:19Z

The solution I went with is not one of the two mentioned above, but a third.

The change that made this work was to pass the package into go test that it should test rather than testing ./....

The package is stackerbuild.io/stacker/./... meaning stacker and all packages under it.

workflow: * add Setup Environment section that defines GOPATH and set GOPATH to <checkoutdir>/../stacker-build/gopath and GOCACHE to <checkoutdir>-build/gopath/gocache. This means that all 'go' operations in subsequent sections will have that variable set. The go toolchain will then correctly re-use anything it can throughout the build. Note these are the same values that use of Make will set both inside and outside the stacker build. (stacker build dir is bind'd into the build environment and Make will set the variable correctly inside also, so the static build will benefit from a cache that the dynamic build did) Note comment in PR project-stacker#443 indicating issues with 'go test ./...' if GOPATH is under the current working directory. * Separate out 'go mod download' from the build. This makes better information available on how long operations things took. Because GOMODPATH is shared, these will used both by the stacker-dynamic and stacker binary builds. Makefile: * Allow caller to use a already-built "level 1 stacker" to build the current source. By setting LEVEL1_STACKER as below, you can avoid building (and re-building) the stacker that is used to build the static stacker. make STAGE1_STACKER=/usr/local/bin/stacker * change 'go test' invocation to explicitly list the stacker package rather than using ./... which can cause problems if GOPATH is under the current working dir. * export GOPATH and GOCACHE (above) * change GO_SRC to only look under pkg/ and cmd/ directories for go source files. Previously it would search the .build directory which is at best a waste of time and often produced errors to stderr about not being able to read directories. * change stacker's dependency from cmd/stacker/lxc-wrapper/lxc-wrapper to cmd/stacker/lxc-wrapper/lxc-wrapper.c . stacker is built inside the stacker environment and the build.yaml file removes the existing lxc-wrapper, so the internal build will never use lxc-wrapper from the external environment. At some point we could use the caching in github actions to re-use a .build/gopath directory to get caching across actions. Signed-off-by: Scott Moser <smoser@brickies.net>

rchincha

lgtm

smoser requested review from rchincha and hallyn as code owners March 14, 2023 16:25

smoser force-pushed the feature/build-separation branch 4 times, most recently from f720a38 to ad8e887 Compare March 14, 2023 17:00

rchincha reviewed Mar 14, 2023

View reviewed changes

.github/workflows/build.yaml Show resolved Hide resolved

smoser force-pushed the feature/build-separation branch from ad8e887 to ece2499 Compare March 15, 2023 15:34

smoser force-pushed the feature/build-separation branch from ece2499 to 73c8e02 Compare March 15, 2023 15:37

smoser force-pushed the feature/build-separation branch from 73c8e02 to 7cf4d97 Compare March 15, 2023 16:00

smoser force-pushed the feature/build-separation branch from 7cf4d97 to 12b3f9c Compare March 15, 2023 16:02

smoser force-pushed the feature/build-separation branch from 12b3f9c to 474bf1a Compare March 15, 2023 16:08

smoser force-pushed the feature/build-separation branch from 474bf1a to 61d6699 Compare March 15, 2023 18:02

rchincha approved these changes Mar 15, 2023

View reviewed changes

rchincha merged commit e78c22b into project-stacker:main Mar 15, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build improvements #443

Build improvements #443

smoser commented Mar 14, 2023

rchincha Mar 14, 2023

smoser Mar 15, 2023

rchincha Mar 15, 2023 •

edited

smoser Mar 15, 2023

smoser Mar 15, 2023

smoser commented Mar 15, 2023

smoser commented Mar 15, 2023

codecov bot commented Mar 15, 2023 •

edited

smoser commented Mar 15, 2023

rchincha left a comment

Build improvements #443

Build improvements #443

Conversation

smoser commented Mar 14, 2023

rchincha Mar 14, 2023

Choose a reason for hiding this comment

smoser Mar 15, 2023

Choose a reason for hiding this comment

rchincha Mar 15, 2023 • edited

Choose a reason for hiding this comment

smoser Mar 15, 2023

Choose a reason for hiding this comment

smoser Mar 15, 2023

Choose a reason for hiding this comment

smoser commented Mar 15, 2023

smoser commented Mar 15, 2023

codecov bot commented Mar 15, 2023 • edited

Codecov Report

smoser commented Mar 15, 2023

rchincha left a comment

Choose a reason for hiding this comment

rchincha Mar 15, 2023 •

edited

codecov bot commented Mar 15, 2023 •

edited