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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve platform support #1161

Closed
wants to merge 30 commits into from
Closed

Improve platform support #1161

wants to merge 30 commits into from

Conversation

jensmeindertsma
Copy link

@jensmeindertsma jensmeindertsma commented Feb 20, 2022

This pull request extends #1130 by implementing the requested changes and making sure things work on Linux ( arm64 -> aarch64 )

Requested changes that were implemented

  • Redundant comments in ./ci/docker/Dockerfile.centos8 were removed
  • $arch_str backwards compatibility: the public download URL's haven't changed at all, the ARM versions now append a -aarch64 suffix while the x86 versions do not have this suffix, to stay backwards compatible. This has also been extended to the volta-install.sh file, which only adds the suffix to the download url if the platform is aarch64.

Unsolved questions

(@charlespierce): Given that the arm64 builds take so long, is there a way we can lock them in the same way as the final Publish release job, so that they only run when we're actually publishing a new version? My worry is that currently this build is part of the CI check for PRs and I don't want to run into a spot where every PR is gated on a build that takes ~2 hours.
This is possible if we split out the ARM build into a separate CI job instead of using a matrix. I've made that change here, but if this is not preferred ( not DRY, but CI files are never really DRY anyway ), let me know.

Most of the changes were made by @vanstinator, and his commits have been preserved in this PR such that he is credited properly for his work. I made this separate PR to speed up the process to getting these changes released.

Reviews would be massively appreciated 馃檹

@jensmeindertsma
Copy link
Author

jensmeindertsma commented Feb 20, 2022

I had to switch the CentOS image to CentOS 7, as that is the only Linux version that is still supported.

@jensmeindertsma
Copy link
Author

jensmeindertsma commented Feb 20, 2022

I just verified locally that the ARM binary works 馃槉

@jensmeindertsma
Copy link
Author

jensmeindertsma commented Feb 20, 2022

My plans after this PR are to add MUSL support to the x86 and aarch64 Linux builds.

@charlespierce now that we have QEMU set up, it would be straightforward to build Volta in a Alpine Docker image and then publish that. Do you think there will be any issues there, regarding glibc dependencies?

@jensmeindertsma
Copy link
Author

I opened #1162 to discuss adding support for Alpine Linux to Volta. I think this PR is ready for review and then merge?

@jensmeindertsma
Copy link
Author

Sorry for the mess of commits, can you squash merge?

@jensmeindertsma
Copy link
Author

And maybe can you first approve the workflow just so we can check everything compiles?

@charlespierce
Copy link
Contributor

Hi @jensmeindertsma thanks for picking these changes up! I approved the CI run. Can you explain a bit more about the switch to CentOS 7? I recognize that CentOS 6 is very old, however using it was an intentional choice to support an older version of libc and thus a broader range of Linux distros.

That currently works in our existing CI pipeline (though there is some flakiness with the package mirror), and I would want to be very deliberate with changing our support base. That would be a breaking change and so would involve releasing Volta 2.0, along with a more in-depth investigation into exactly how the support would change.

@jensmeindertsma
Copy link
Author

Absolutely! I switched to CentOS 7 as there were some issues with the package mirrors. I'll give them another shot now. I'll re-add the CentOS 6 and 7 images, and can you then approve the workflow again as soon as those changes are up?

@jensmeindertsma
Copy link
Author

@charlespierce thanks for the super quick response by the way! I just added back CentOS 6, I don't want to cause any breaking change. Could you approve the workflow again?

@jensmeindertsma
Copy link
Author

@jensmeindertsma
Copy link
Author

But wait, the new aarch64 binaries don't actually work?

@jensmeindertsma
Copy link
Author

@charlespierce could you elaborate on why we build for both OpenSSL 0.1.0 and 1.1.0?

@jensmeindertsma jensmeindertsma marked this pull request as draft February 22, 2022 20:02
@charlespierce
Copy link
Contributor

@jensmeindertsma We build against 3 different OpenSSL versions: OpenSSL 1.0.0, OpenSSL 1.1.0, and OpenSSL 1.0 on CentOS. Those three all have different soname dynamic libraries and aren't directly binary compatible with each other. We build each of those to support a wide range of target Linux distros and versions.

@jensmeindertsma
Copy link
Author

I'm closing this now and I'll redo this work once #1164 is merged. That makes this waaaay easier

@charlespierce
Copy link
Contributor

Notably: OpenSSL doesn't (or at least didn't, not sure about 3.0) follow SemVer, so OpenSSL 1.0.0 and 1.1.0 are incompatible with each other, despite being only a minor version bump apart.

@jensmeindertsma jensmeindertsma deleted the support-alpine-linux branch February 22, 2022 20:28
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

3 participants