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

*: add cross-build, CI job, update to libseccomp 2.5.2 #3197

Merged
merged 4 commits into from
Sep 22, 2021

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Sep 3, 2021

This implements cross-build for make release, moving the build into a
container. This way we can support arm, arm64, ppc64, and whatnot.

  • script/seccomp.sh: separate out of script/release.sh, amend to support
    cross-compile and save needed environment variables to a file.

  • Dockerfile: add installing libseccomp from source, as this is needed
    for release builds.

  • script/release.sh: amend to support more architectures in addition to
    the native build. Additional arches can be added by specifying
    -a <arch> argument (can be specified multiple times), or
    make RELEASE_ARGS="-a arm64 -a ppc64le" release if called via make.

  • Makefile: move release target to localrelease, add release
    target to build via the Dockerfile. This is done because most distros
    (including Fedora and openSUSE) lack cross-glibc, which is needed to
    cross-compile libseccomp.

  • Makefile: remove cross and localcross targets, as this is now done
    by the release script.

  • .github/workflows/validate.yum: amend the release CI job to cross-build
    for supported architectures, remove cross job.

@kolyshkin kolyshkin force-pushed the release-arm64 branch 3 times, most recently from d452794 to b5301ec Compare September 3, 2021 22:12
@kolyshkin kolyshkin marked this pull request as draft September 3, 2021 22:12
@kolyshkin kolyshkin changed the title release: add arm64 build [WIP] release: add arm64 build Sep 3, 2021
@kolyshkin kolyshkin force-pushed the release-arm64 branch 2 times, most recently from 16cf452 to feb385f Compare September 3, 2021 22:19
script/release.sh Outdated Show resolved Hide resolved
@kolyshkin kolyshkin force-pushed the release-arm64 branch 5 times, most recently from 59b302e to f1be556 Compare September 6, 2021 19:34
@kolyshkin
Copy link
Contributor Author

OK I made it working...

...alas not for opensuse (or Fedora, for that matter). Problem is, only Debian (and Ubuntu, to some extent) provide good cross-compilation support. For opensuse in particular, this is nicely summarized by this statement (from https://software.opensuse.org/package/cross-aarch64-gcc11):

Note this is only useful for building freestanding things like the kernel since it fails to include target libraries and headers.

It's the same for Fedora (from rpm -qi gcc-aarch64-linux-gnu output):

Only building kernels is currently supported. Support for cross-building
user space programs is not currently provided as that would massively multiply
the number of packages.

Ubuntu seems to fully support at least arm64 (as shown by validate/release CI job in this PR), but does not support some other architectures (so we have to use Docker for validate/cross CI job).

To sum it up -- building runc.amd64 it's ready and working (in fact you can download it from this PR's CI -- go to Checks tab, choose validate, and scroll down looking for Artifacts, in there you can find a zip with the binaries), but only for Ubuntu/Debian.

We can change the release process to use the binaries built in CI, but it's a big change.

@cyphar @AkihiroSuda WDYT?

@kolyshkin kolyshkin changed the title [WIP] release: add arm64 build release: add arm64 build Sep 6, 2021
@cyphar
Copy link
Member

cyphar commented Sep 7, 2021

@kolyshkin Since we only have one dependency (libseccomp) which we build ourselves, I think it might be possible for us to do it -- we should only require an architecture-appropriate compiler and glibc. But if that isn't possible (or is too fragile / annoying) we can switch to building cross-architecture binaries in CI.

The only issue is that I wouldn't be able to automatically sign them as part of the release (I can obviously manually sign them afterwards but it'll be a bit more manual work than the release process currently uses). I wonder if we could just script the whole release process with the GitHub API (push the tag, wait for the CI to build the cross-arch artefacts, download, sign, then upload to the release). Then again, how much time would we be saving over the manual method? 😅

@AkihiroSuda
Copy link
Member

The only issue is that I wouldn't be able to automatically sign them as part of the release (I can obviously manually sign them afterwards but it'll be a bit more manual work than the release process currently uses). I wonder if we could just script the whole release process with the GitHub API (push the tag, wait for the CI to build the cross-arch artefacts, download, sign, then upload to the release). Then again, how much time would we be saving over the manual method? 😅

SGTM, but I don't think signing has to be automated.

If we want to sign the binary on GHA, at least we will have to create a new GPG key, because I don't want you to leak your existing GPG key to GHA secrets 😄

@kolyshkin
Copy link
Contributor Author

Since we only have one dependency (libseccomp) which we build ourselves, I think it might be possible for us to do it -- we should only require an architecture-appropriate compiler and glibc.

which, sadly, neither Fedora nor openSUSE has -- they only provide gcc and binutils.

aarch64-glibc is available for both Debian and Ubuntu, though.

Automating the release process in CI seems like a bigger project that I can take on at the moment. If anyone can help that'd be awesome.

This PR can go through, though, once I'll add an option to script/release.sh to specify additional architectures.

@cyphar
Copy link
Member

cyphar commented Sep 9, 2021

@AkihiroSuda

SGTM, but I don't think signing has to be automated.

If we want to sign the binary on GHA, at least we will have to create a new GPG key, because I don't want you to leak your existing GPG key to GHA secrets smile

Haha, I meant to say that we have a hypothetical "release.sh" script I run locally which:

  1. Pushes the release tag and waits for the CI to run and build the release artefacts.
  2. Downloads the release artefacts.
  3. (Optionally) verifies that the match the locally built version (for amd64).
  4. Signs everything.
  5. Creates a new draft release and uploads the artefacts and signatures via the GitHub API.
  • This would also allow us to automate the annoying copy-pasting I have to do for the LGPL license notice.
  1. (Optionally) Opens my web browser to the draft release.

For what it's worth we probably should have a shared runc maintainers key (or maybe a runc.keyring stored in the repository would be sufficient) so I'm not the only one who is able to sign releases. I'm also not a fan of the idea of having signing be automated with a CI action -- it somewhat defeats the purpose of signing (if someone has the ability to publish a release they'd have access to the signing keys too).

@cyphar
Copy link
Member

cyphar commented Sep 9, 2021

@kolyshkin

Since we only have one dependency (libseccomp) which we build ourselves, I think it might be possible for us to do it -- we should only require an architecture-appropriate compiler and glibc.

which, sadly, neither Fedora nor openSUSE has -- they only provide gcc and binutils.

Damn. Yeah guess we'll need to figure out some other solution for signing -- but this PR should be enough for the moment (I can manually add the artefacts for the next few releases until we have a better release script).

@kolyshkin kolyshkin marked this pull request as ready for review September 9, 2021 20:12
@kolyshkin
Copy link
Contributor Author

This is no longer a draft and ready to go in. Used by CI, and can be used locally as well (make RELEASE_ARGS="-a arm64" release) as long as prerequisites are met. @cyphar PTAL

@kolyshkin kolyshkin changed the title release: add arm64 build script/release.sh: add arm64 cross-build (and enable it in CI) Sep 9, 2021
@kolyshkin
Copy link
Contributor Author

Alternatively (and I'm surprised it took me this long to think of this) -- we could just always build the binaries inside a container

You and me both 😆

We already have a Debian-based image that can be used for testing and cross-compilation. It's easy to reuse it for building release binaries... let me see...

@h-vetinari
Copy link

Alternatively (and I'm surprised it took me this long to think of this) -- we could just always build the binaries inside a container

You and me both 😆

(with nothing but deep respect for y'all's work) this is hilarious. 😄

@kolyshkin kolyshkin changed the title script/release.sh: add arm64 cross-build (and enable it in CI) *: add cross-build, CI job, update to libseccomp 2.5.2 Sep 13, 2021
@kolyshkin kolyshkin force-pushed the release-arm64 branch 4 times, most recently from a8deec7 to eaec37f Compare September 13, 2021 23:06
@kolyshkin
Copy link
Contributor Author

OK, this is finally ready, you can use

make RELEASE_ARGS="-a arm64 -a armel -a armhf -a ppc64le" release

locally. It takes 5 minutes on CI (with cache, or 10 minutes without), and about 1 minute on my laptop (using podman, with all layers already cached).

You can still use script/release.sh directly as before (or make localrelease via Makefile). I am not sure if I should remove building libseccomp from it, as currently it relies on seccomp being pre-built).

Overall I think this is a big ugly now, and can be simplified and improved (I am pretty sure @thaJeztah can think of some Dockerfile improvements, for one thing), but I've already spent much more than I wanted on it.

@kolyshkin
Copy link
Contributor Author

Note that I have removed cross CI job as the release job supersedes it. Will remove cross from the list of required jobs if/after this is merged.

@kolyshkin kolyshkin added this to the 1.1.0 milestone Sep 14, 2021
@kolyshkin
Copy link
Contributor Author

@cyphar @AkihiroSuda PTAL

script/seccomp.sh Outdated Show resolved Hide resolved
cyphar
cyphar previously approved these changes Sep 20, 2021
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM. Now to automate the rest of the release process. (I'll work on that sometime soon. 😉)

1. The seccompagent target it built in the same way as others in contrib,
   so there is no need to have a separate rule.

2. Mark seccompagent as phony, because it is (it rarely happens, but I
   actually just had an issue because this was absent).

3. Add seccompagent binary to clean target.

Fixes: e21a9ee

Signed-off-by: Kir Kolyshkin <[email protected]>
There is no need to have a static version of recvtty and/or sd-helper
binary.

This speeds up script/release.sh a bit.

Signed-off-by: Kir Kolyshkin <[email protected]>
This implements cross-build for "make release", moving the build into a
container. This way we can support arm, arm64, ppc, and whatnot.

* script/seccomp.sh: separate out of script/release.sh, amend to support
  cross-compile and save needed environment variables to a file.

* Dockerfile: add installing libseccomp from source, as this is needed
  for release builds.

* script/release.sh: amend to support more architectures in addition to
  the native build. Additional arches can be added by specifying
  "-a <arch>" argument (can be specified multiple times), or
  "make RELEASE_ARGS="-a arm64" release" if called via make.
  All supported architectures can be enabled via "make releaseall".

* Makefile: move "release" target to "localrelease", add "release" and
  "releaseall" targets to build via the Dockerfile. This is done because
  most distros (including Fedora and openSUSE) lack cross-glibc, which is
  needed to cross-compile libseccomp.

* Makefile: remove 'cross' and 'localcross' targets, as this is now done
  by the release script.

* .github/workflows/validate.yum: amend the release CI job to cross-build
  for supported architectures, remove cross job.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Contributor Author

Addressed review comments from @cyphar; PTAL

Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM -- thanks for working on this!

@cyphar
Copy link
Member

cyphar commented Sep 21, 2021

I removed cross from the list of required checks since this PR removes the job entirely.

@mrunalp mrunalp merged commit 147ad56 into opencontainers:master Sep 22, 2021
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.

5 participants