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

Update to go1.16 and split releaser to separate builds on Linux, macOS, and Windows #599

Merged
merged 15 commits into from
Mar 11, 2021

Conversation

tomelm
Copy link
Contributor

@tomelm tomelm commented Feb 21, 2021

Reviewers

r? @suz-stripe
cc @stripe/developer-products @pepin-stripe @gracegoo-stripe

Summary

I can't figure out how to only diff on my other PR so this contains some of the same commits as #598. Do not merge until #598 is mergeable.

Before we can use the embed feature in go1.16 we need to make sure our build processes compile against 1.16 instead of 1.13.x.

We've had to use the mailchain goreleaser Docker image to make sure that macOS compilation works correctly (#439). Unfortunately, that's based on dockercore/golang-cross which hasn't updated past go1.13 and doesn't seem like they're going to in the near future (see docker/golang-cross#54 and docker/golang-cross#64).

This PR splits our build and release into two flows:

  1. for macOS
  2. for everything else

Building explicitly for macOS will make sure we don't have the DNS-related issues in #439, but also makes it easier for us to build for Apple Silicon (including building a universal binary, which requires lipo) and signing/notarizing the binary (which requires some macOS-specific tooling from Xcode). I'm not sure if we should just move everything to macOS at the risk of introducing potential other compatibility issues for Linux or Windows, but having split workflows doesn't seem like a bad idea at the moment.

Plus, overall this will reduce our build dependency chain on third parties, giving us more control.

edit: in case it's not obvious I just got one of the M1 laptops and have been playing around with the ARM compilation 😆

@CLAassistant
Copy link

CLAassistant commented Feb 21, 2021

CLA assistant check
All committers have signed the CLA.

@stripe-ci stripe-ci assigned tomelm and unassigned suz-stripe Mar 7, 2021
Copy link

@themar7777 themar7777 left a comment

Choose a reason for hiding this comment

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

Windows/remove

@tomelm
Copy link
Contributor Author

tomelm commented Mar 7, 2021

ptal @suz-stripe
cc @tjfontaine-stripe @pepin-stripe

I updated this PR over the other (#598). I started working to pull the Dockerfile in and as I got deeper into it, there were some errors occurring around installing docker-ce. There are workarounds (see this) but I was getting increasingly worried about how many work arounds we were introducing, it felt like it was growing very brittle.

I figure split build processes might be the least risky option long term. I've been testing it in another repo (here) and the release "worked" (minus a few failures; the mac-build failure was because it tried to upload to the stripe homebrew repo, the windows issue looks like it was rate limited). The release itself actually worked and only created one released for the version tag (see v0.0.10).

I did split this out to mac, windows, and linux builds. Instead of one checksums file, I created one for each platform (there were overwrite issues). arm actually does not work yet because the macOS 11.0 environment is private at the moment (see actions/runner-images#2486).

GORELEASER_GITHUB_TOKEN: ${{ secrets.GORELEASER_GITHUB_TOKEN }}
DOCKER_USERNAME: ${{ secrets.DOCKER_USERNAME }}
DOCKER_PASSWORD: ${{ secrets.DOCKER_PASSWORD }}
GITHUB_TOKEN: ${{ secrets.GORELEASER_GITHUB_TOKEN }}
-
name: Upload to Bintray
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like jobs run in parallel by default so we probably need to split that in its own job and have it depend on both goreleaser jobs https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobs

- CXX=x86_64-w64-mingw32-g++
main: ./cmd/stripe/main.go
goos:
- windows
Copy link
Contributor

Choose a reason for hiding this comment

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

is that supposed to be windows since this is the config file for the linux build?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, need to remove this, good catch!

.github/workflows/release.yml Show resolved Hide resolved
Copy link
Contributor

@pepin-stripe pepin-stripe left a comment

Choose a reason for hiding this comment

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

lgtm!

@tomelm tomelm changed the title [wip][do not merge] Update releaser to separate builds on Linux and macOS Update to go1.16 and split releaser to separate builds on Linux, macOS, and Windows Mar 9, 2021
@tomelm tomelm removed their assignment Mar 9, 2021
@tomelm
Copy link
Contributor Author

tomelm commented Mar 9, 2021

This is ready! Most recent test run is here: https://github.com/tomelm/stripe-cli-temp/actions/runs/634468413. Note that the test run doesn't include some of the follow up steps such as Homebrew, Docker, Bintray, or VirusTotal. I can't easily test those but none of those steps or code changed here so should be okay!

Copy link
Contributor

@suz-stripe suz-stripe left a comment

Choose a reason for hiding this comment

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

This looks great and is a step in the right direction long term for our release process.

I initially had concerns around what would happen if three different goreleaser release runs would cause issues for the GitHub release step with regards to the tag, the changelog and the archives attached but looking at your practice run repo it looks like it seems to work fine. I am delighted but also confused as to how goreleaser handles this so gracefully. I'm guessing it checks for an existing tag / published release and just pushes the builds to an existing release if it finds one? Perhaps GitHub also smoothes this over with idempotence in their API.

As for the Bintray / Virustotal steps, I assume that the GitHub release archives needed should be available as long as these upload steps are run in the right os context.

Going forward, I recommend that we think about building dry run abilities into our scripts so that we can easily test our scripts that are run outside of goreleaser. For example, our bintray and virustotal scripts could check for a dry run env var or flag, and run all steps except for uploading. This way we can ensure the other important parts of the script work such as ensuring env vars are correctly supplied, the script has the right permissions and dependencies installed, and that the GitHub release download URLs resolve correctly to files so we can spot issues with our goreleaser config changes.

This dry run could also extend to running the release workflow against a temp repo for cli / brew / scoop etc. Just some thoughts! 💭

.github/workflows/release.yml Show resolved Hide resolved
.github/workflows/release.yml Show resolved Hide resolved
@tomelm
Copy link
Contributor Author

tomelm commented Mar 9, 2021

Agree with your feedback (and concerns!). The dry-run flag sounds like a great idea that we should look into in the future, there's still a lot of friction trying to manually verify build processes like this.

I think my ideal extension of this in the future is that each OS only builds the binary and we have subsequent steps that handle things like writing the changelog, uploading the release, getting it it to bintray and virus total, etc but that's a bit more complicated (especially since I couldn't find a way to run goreleaser without running a build, I wanted to an additional step to write the changelog afterwards).

@tomer-stripe tomer-stripe merged commit dbc4d53 into stripe:master Mar 11, 2021
@tomelm tomelm deleted the tomer/arm branch March 13, 2021 15:08
az777777 pushed a commit to az777777/stripe-cli that referenced this pull request Jun 22, 2021
…S, and Windows (stripe#599)

* Update to go 1.16 and update dependencies

* Update linter and fix new linting issues

* Test against go.1.16

* Run mod tidy

* Start splitting the goreleaser flow to mac and everything else

* Remove the golang cross compile steps

* Update goreleaser scripts for separate build paths

* Remove some unneeded steps temporarily

* Split builds per platform

* Add missing docker, nfpms, and scoop steps

* Apply yaml formatter for consistency

* Move build files into a folder and make the release steps consistent

* Apply consistent formatting

* Add fetch depth to mac and windows
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.

None yet

6 participants