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.

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

GH-103 - Refactor build script #105

Merged
merged 12 commits into from
Nov 24, 2020
Merged

GH-103 - Refactor build script #105

merged 12 commits into from
Nov 24, 2020

Conversation

davidalpert
Copy link
Contributor

From #103:

Noted items within the build scripts that should be cleaned up:

  • support for building macos in utils/ci.go
  • root makefile should call utils/ci.go
  • ci.go test creates a stray logfile in the build directory which should be a temp file

TL;DR I was unable to locate any stray logfiles from running utils/ci.go test, and there are 2 make tasks which don't yet go through utils/ci.go.

Description of changes

While working on this I added godbledger-darwin and godbledger-windows make targets following the pattern of the godbledger-linux-* targets. This partially addresses #100 and #101

Here is a screenshot of running ./build/bin/godbledger-darwing-10.6-amd64 (the output from invoking make godbledger-darwin) running on MacOS 10.15.6 Catalina:
image

Here is a screenshot of running ./build/bin/godbledger-windows-4.0-amd64.exe (the output from invoking make godbledger-windows) running on a Windows Server 2016 VM:
image

The root makefile now calls utils/ci.go for almost every target. Notable exceptions are:

target notes
linux this generates 3 binaries with simple suffixed names in a versioned folder (e.g. ./release/godbledger-linux-x64/vlatest/godbledger along with ledger_cli and reporter in the same folder) instead of the suffixed names in a standard bin folder generated by the xgo-based build (e.g. ./build/bin/godbledger-linux-arm63 and no ledger_cli nor reporter)
cert this is not represented in utils/ci.go today, though certainly could be

I have updated the make release target to depend on the godbledger-linux-arm godbledger-darwin godbledger-windows targets, which means make release now uses the xgo docker container to build linux, mac, and windows binaries.

At this point I need some feedback/direction.

Request for Feedback

Given the following:

  • make release:
    image
  • make / make all:
    image
  • make linux:
    image

Which of these patterns do we want to keep and are there any that we can remove?

For instance, simple ./build/bin output of all three tools makes sense as an easy way to run local code and tests when developing.

Unsuffixed output of all three tools in a versioned folder makes sense (like make linux) but doesn't seem to be the norm for the set of xgo-based make targets.

this completes a local development story where 'make' will build
all three to ./build/bin for whatever platform you run it on by
calling through to 'utils/ci.go install' and 'make test' calls
through to 'utils/ci.go test'
the xgo-based build targets appear to build inside an xgo
container which can fail a build if go.mod and go.sum are
not up-to-date:

    go: writing go.sum: open /ext-go/1/src/go-repo/go.sum747278511.tmp: read-only file system
    2019/09/11 15:45:50 Failed to cross compile package: exit status 1.

for more detail see techknowlogick/xgo#36
addresses the 'utils/ci.go xgo' part of darcys22GH-100 and darcys22GH-101
@davidalpert
Copy link
Contributor Author

@darcys22 just read your comment on #101

I hadnt been able to get xgo working properly. Its possibly a solution however might need some futher research on how other projects are building their releases.

maybe that is why the xgo-based targets are not building all 3 apps? because you ran into problems with using xgo?

I had some problems also but seem to have worked through them and now seem to have cross-compiled builds working using the xgo docker images as local build agents.

I don't yet understand the purpose of xgo over what go tooling provides natively, however, as I have previously used a simple bash script which loops over GOOS and invokes go build with GOOS and GOARCH flags. That seems to be sufficient to generate linux, macos, and windows compatible binaries on my MacOS laptop and on our linux-based jenkins build agents.

@davidalpert
Copy link
Contributor Author

perhaps with the github actions matrix approach demonstrated here there is room to simplify the makefile and utils/ci.go both?

@davidalpert
Copy link
Contributor Author

Github Actions may solve cross-compilation through a matrix of parallel docker build environments rather than one xgo pattern; check out the action results attached to PR #106

@darcys22
Copy link
Owner

This is awesome! appreciate the work. Wouldn't worry about the stray logfile, it might be the linter but I'll look into that separately.

Getting rid of the 'release' directory and moving everything to 'build/bin' is my preference. The make linux call can be deleted as make all would be the default now.

I'm not 100% sold on xgo, it was supposed to be useful for other builds such as arm but it seemed like the devs might not be fully behind it anymore. Would be happy to be guided by your thoughts here considering you actually got it working. If using github actions is easier id be for it.

Also planning on coming back to this build script in the not too distant future because I want to get debs working with #108. If this works locally for you im okay merging it

@davidalpert
Copy link
Contributor Author

I got xgo working and it's kind of cool in that it spins up a docker container and spits out binaries, but I'm not sure that it's necessary as where I work we do the same thing locally without a docker container using the go native tools and a shell script.

this does work locally for me however so we could leave xgo for now and defer a decision to remove it.

removing ./release makes sense but moving everything to ./build/bin leaves me with another question:

  • it's convenient to run make and get a simply named binary in a predictable location like ./build/bin/godbledger; makes building and running the local version of the code very straightforward.
  • that ease doesn't mesh well with multiple targets (i.e. including version numbers, GOOS names, folders, etc makes it all more complicated)

the pattern we've settled on where I work is actually that we cross-build using a shell script to ./bin/{GOOS}/appname so for example we might generate:

  • ./bin/darwin/godbledger
  • ./bin/linux/godbledger
  • ./bin/windows/godbledger
    which still has the benefit of a simple and predictable filepath and name to run the local code easily and also makes it fairly easy to grab everything under ./bin (or in this case ./build/bin) and package or ship it.

another option might be to keep ./build/bin/{app} as the simple binaries built for the current OS and add another folder under build like ./build/dist/v{version}-{GOOS}/ so that all the built output is under the ./build folder and ./build/bin is for local use while ./build/dist is for packaging/distribution.

@darcys22
Copy link
Owner

Yeah leave xgo for now if its working.

Could we change the build scripts it to be ‘/bin/{GOOS}/appname’ same as your work. To be honest is all an improvement over what was previously and dont have too strong of an opinion on the file structure as long as its consistent.

as I understand it this is more idomatically go
especially in a cross-compile scenario, since
'go install' only makes sense for GOOS-native
binaries; cross-compiled binaries can be 'built'
but not 'installed' on a local machine due to
OS/ARCH mismatches
@davidalpert
Copy link
Contributor Author

davidalpert commented Nov 22, 2020

I haven't dropped this, but it is taking me a while because I want to make sure that I don't break the ability to release the same builds which have been released to-date.

turns out the go-sqlite3 dependency requires cgo and CGO_ENABLED=1 when building; this complicates cross-compilation beyond what we manage at work due to requiring valid C/C++ tool chains for the target architecture be installed, in the path, and configured correctly.

I think I understand now why the etherium project uses xgo (using docker to build against native dependencies rather than install all the various x-platform dependencies on a single system) and they seem to have a stable cross-compile story so I'm reviewing that as well; no need to reinvent this wheel.

I enjoy working through this kind of build/release plumbing so happy to contribute.

davidalpert and others added 4 commits November 22, 2020 21:06
the semantics of 'make' will default to build a native
version of all the apps into ./build/bin/native for
local development and testing; this places the native
binaries at a consistent location across environments.

another target 'make release' can be used to kick
off cross-compilation and prepare the full suite
of release artifacts
- introduces a new release process behind a feature flag

    - utils/make-release.sh should work as before

    - release_pattern=xgo utils/make-release.sh will
      use xgo/docker-based builds

- the linting cache dir is moved into ./build/.cache/

- xgo-based build-cross targets now build all apps and
  dump all build output into ./build/dist/{target}/
  where {target} is the xgo cross-compilation target

- utils/ci.go's 'xgo' subcommand is updated to:

  - accept only a single xgo compilation target;

  - build all three apps (godbledger, ledger_cli, reporter)
    one at a time (xgo doesn't seem to allow building
    multiple outputs from a single container, though maybe
    I just haven't found the correct syntax)

  - rename the binary output to remove the build target
    suffix (e.g. rename 'reporter-linux-arm-5' to 'reporter')
if it's not set in the environment, assume it is /home/vagrant/go/
@davidalpert
Copy link
Contributor Author

davidalpert commented Nov 23, 2020

made some progress:

  • can still run make-release.sh like before (assuming it worked before, I had to build some linux VM and juggle build tools to get the right toolchains installed):
    ./utils/make-release.sh
    
  • can use a feature-flag to opt-in to the xgo-based release steps which require docker (readme updates to set up docker are on my TODO list for this PR):
    release_pattern=xgo ./utils/make-release.sh
    

release zipfiles are still written to ./release/ as before and should contain the same contents as before unless the release_pattern=xgo feature flag is enabled; then the contents of the zipfiles are simplified to contain unsuffixed binary names (e.g. just godbledger, ledger_cli, and reporter)

using the makefile:

  • simple make calls make build-native and builds native binaries (native to the host GOOS/GOARCH) to ./build/bin/native/
  • make test depends on make build-native and uses native binaries for the integration test
  • make build-cross uses the xgo/docker-based commands to cross-build a range of supported binaries, all written to ./build/dist/{target}/

using utils/ci.go:

  • I renamed the install subcommand to build as installing doesn't make sense when it is passed an os or arch flag to cross-build using local tools (i.e. not using xgo/docker)
  • I left the ./build/ folder for now as it collects all the build output
  • native binaries are consistently written to ./build/bin/native/ to differentiate them from locally cross-built binaries which are built to ./build/bin/{os}-{arch}/

tomorrow I can update the README with some of these notes and instructions on installing docker to use xgo

@darcys22
Copy link
Owner

This is gold :). The make-release.sh did work previously and would definitely like to keep the one liner command for creating releases when all is finished. If you have a better method than the script after this however im open to changing the process

@davidalpert
Copy link
Contributor Author

xgo seems to have a limitation that you can only send it a single package at a time, so to build all three tools against one architecture that spins up the same docker image three times. not terribly efficient, but seems to be a stable cross-build approach.

if that release_pattern=xgo feature-flag works for you to use xgo it could easily be extended to build for all the supported xgo targets, including darwin and windows.

the only other option I wondered about was using github actions to simply run build-native on each host architecture, but xgo has the benefit of being able to run locally and not hanging the release builds on github's action hosts as a dependency.

in any case this definitely meets my needs to run make and get native builds on darwin and ubuntu, as well as standardizing the xgo pattern, so if it works in your environment also I'd say shipit.

there's one other simplification I can think of around how the version number is handled but that can wait until a second follow-up PR.

will update the README shortly.

using the xgo pattern to cross-compile requires installing
a version of Docker Engine to host the xgo build containers

these readme updates point the way towards Docker's platform-
specific instructions on how to install Docker Engine on
various flavours of linux, as well as the docker-for-desktop
builds for MacOS and Windows

hopefully this can help others get up and running to build
and run godbledger code locally and assist with cross-compiling
in the future.
@darcys22
Copy link
Owner

Just tested this all locally and works well for me. Very impressed with how its all come together :) :)

Unless you have anything else to add I'm ready to merge this

@davidalpert
Copy link
Contributor Author

merge away. I will offer another small idea for discussion in a subsequent PR around how the version number is passed around.

@davidalpert davidalpert mentioned this pull request Nov 24, 2020
@darcys22 darcys22 merged commit 26995bb into darcys22:master Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants