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

build: WASM_ENABLED=1 for all platforms #3416

Merged

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Apr 28, 2021

Notes:

  • If there are other users of the 'build-windows' make target they would
    likely be annoyed by the change that's now apt-get'ting packages

  • We could build a builder image instead of installing the package every
    time.

  • ci-go-*: run as root now, so we're able to install the packages for
    windows.

  • tests: skip tests that depend on not being run as root when root. The
    change to ci-go-* makes that necessary; the impact is rather limited
    right now. We can reconsider if there are more tests depending on not
    being run as root.

  • build: add '-buildmode=exe' to GOFLAGS

    Primarily for the windows build, but I don't think it should be wrong
    for the others either:

    cmd/link: "x86_64-w64-mingw32/bin/ld.exe: Error: export ordinal too large" after upgrading to Go 1.15 golang/go#40795

    See https://golang.org/cmd/go/#hdr-Build_modes:

    -buildmode=exe
    Build the listed main packages and everything they import into
    executables. Packages not named main are ignored.

  • go: bump to 1.16.3, keep compat with 1.15

    See https://golang.org/ref/mod#go-mod-file-go about the go directive in
    go.mod:

    A go directive indicates that a module was written assuming the semantics
    of a given version of Go.

  • build: update go module related env vars

    With 1.16, https://blog.golang.org/go116-module-changes,

    The go command now builds packages in module-aware mode by default.

    Also, since we've added the go 1.15 directive to go.mod, we can drop
    all -mod=vendor flags, https://golang.org/ref/mod#go-mod-file-go,

    At go 1.14 or higher, automatic vendoring may be enabled. If the file
    vendor/modules.txt is present and consistent with go.mod, there is no
    need to explicitly use the -mod=vendor flag.

  • build: replace build-docker by build-linux

    Turns out the binaries were identical now.

  • workflow: adapt github workflows

  • build: override docker id/gid in 'image' target, to keep existing
    behaviour.


References:

  1. https://dh1tw.de/2019/12/cross-compiling-golang-cgo-projects/
  2. Improve --target=wasm experience when runtime not supported #3264: if all our release binaries have wasm enabled, much less people will run into the panic.

@srenatus srenatus force-pushed the sr/build/more-wasm-enabled branch 5 times, most recently from cac023a to a13db96 Compare April 28, 2021 10:07
@srenatus srenatus changed the title build: WASM_ENABLED=1 for all platforms build: WASM_ENABLED=1 for all platforms (except darwin) Apr 28, 2021
Copy link
Member

@tsandall tsandall left a comment

Choose a reason for hiding this comment

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

Just questions.

.github/workflows/pull-request.yaml Show resolved Hide resolved
.go-version Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@srenatus srenatus force-pushed the sr/build/more-wasm-enabled branch 2 times, most recently from a9e3345 to f9700ca Compare April 28, 2021 11:48
@srenatus srenatus changed the title build: WASM_ENABLED=1 for all platforms (except darwin) build: WASM_ENABLED=1 for all platforms Apr 28, 2021
@srenatus srenatus force-pushed the sr/build/more-wasm-enabled branch 2 times, most recently from d0bd2bc to d3260f1 Compare April 28, 2021 12:24
@srenatus srenatus marked this pull request as ready for review April 28, 2021 12:57
Copy link
Contributor Author

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Some notes

.github/workflows/post-merge.yaml Outdated Show resolved Hide resolved

release-build-darwin:
name: Release Build (darwin)
runs-on: macos-latest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👉 https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idruns-on

macos-latest is 10.15, not big sur. But the binaries work on the latter, too.

@srenatus
Copy link
Contributor Author

Downloading the gcc for mingw32 on the fly still lets the linux+windows build finish under 3 minutes. I'd say we look into a builder image if this proves to be brittle or annoying.

@tsandall
Copy link
Member

Downloading the gcc for mingw32 on the fly still lets the linux+windows build finish under 3 minutes. I'd say we look into a builder image if this proves to be brittle or annoying.

I guess as long as no one is using build-windows in their dev environment, that's fine.

@srenatus srenatus force-pushed the sr/build/more-wasm-enabled branch 8 times, most recently from a100d1f to 71c3182 Compare April 29, 2021 08:21
@srenatus
Copy link
Contributor Author

Squashed and ready to go. 🏁

@srenatus
Copy link
Contributor Author

Splitting the Linux/Windows build doesn't make a huge difference, but it won't hurt, I think:

Before

Screenshot 2021-04-29 at 13 23 50

After

Screenshot 2021-04-29 at 13 24 01

☝️ The feedback for build/linux and npm-opa-wasm will be a bit quicker. 🚤

tsandall
tsandall previously approved these changes Apr 30, 2021
Makefile Show resolved Hide resolved
build/ensure-windows-toolchain.sh Show resolved Hide resolved
env:
TELEMETRY_URL: ${{ secrets.TELEMETRY_URL }}
run: make release-local
- name: Download release binaries
Copy link
Member

Choose a reason for hiding this comment

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

Just for my own understanding...whereas before we would build the OPA binaries here, now they will be built above by release-build and release-build-darwin and then downloaded here. Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup!

@srenatus srenatus force-pushed the sr/build/more-wasm-enabled branch from b94f0ba to 416b3d9 Compare May 4, 2021 08:20
@srenatus srenatus requested a review from tsandall May 4, 2021 08:20
@srenatus srenatus force-pushed the sr/build/more-wasm-enabled branch 2 times, most recently from 6401c9c to 600ce9b Compare May 4, 2021 08:28
tsandall
tsandall previously approved these changes May 5, 2021
Notes:
- If there are other users of the 'build-windows' make target they would
  likely be annoyed by the change that's now apt-get'ting packages
- We could build a builder image instead of installing the package every
  time.
- ci-go-*: run as root now, so we're able to install the packages for
  windows.
- tests: skip tests that depend on not being run as root when root. The
  change to ci-go-* makes that necessary; the impact is rather limited
  right now. We can reconsider if there are more tests depending on not
  being run as root.

- build: add '-buildmode=exe' to GOFLAGS

  Primarily for the windows build, but I don't think it should be wrong
  for the others either:

  golang/go#40795

  See https://golang.org/cmd/go/#hdr-Build_modes:

  > -buildmode=exe
  > Build the listed main packages and everything they import into
  > executables. Packages not named main are ignored.

- go: fix version as 1.16.3

  This was discussed in this PR, but not the other one. We'd rather keep
  this an exact match.

- build: update go module related env vars

  With 1.16, https://blog.golang.org/go116-module-changes,

  > The go command now builds packages in module-aware mode by default.

  Also, since we've added the `go 1.15` directive to go.mod, we can drop
  all -mod=vendor flags, https://golang.org/ref/mod#go-mod-file-go,

  > At go 1.14 or higher, automatic vendoring may be enabled. If the file
  > vendor/modules.txt is present and consistent with go.mod, there is no
  > need to explicitly use the -mod=vendor flag.

- build: replace build-docker by build-linux

  Turns out the binaries were identical now.

- workflow: adapt github workflows
- build: override docker id/gid in 'image' target, to keep existing
  behaviour.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
split linux and windows to not wait for the windows build to finish
before starting the npm-opa-wasm tests.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Fixes open-policy-agent#3264.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
Copy link
Contributor Author

srenatus commented May 5, 2021

This was approved, and rebased -- will squash and merge when green.

@srenatus
Copy link
Contributor Author

srenatus commented May 5, 2021

This PR had a spurious "waiting for status to be reported" right from the start, but it's actually all green now: https://github.com/open-policy-agent/opa/actions/runs/813799167

@srenatus srenatus merged commit f23fb0f into open-policy-agent:main May 5, 2021
@srenatus srenatus deleted the sr/build/more-wasm-enabled branch May 5, 2021 15:06
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