-
Notifications
You must be signed in to change notification settings - Fork 123
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
Feat: Upgrade to Ruby 3.1.2 / Support ARM64 OSX/Linux #132
Conversation
Thanks! I'll have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's my preliminary review. I haven't actually tested this so I don't know whether it actually works.
I also see that your comment mentions Github Actions but there's no actual Github Actions workflow. But luckily I have both an M1 Macbook and an x86 one so I can build for both manually if needed.
@@ -230,6 +230,11 @@ fi | |||
|
|||
header "Preparing Ruby source code..." | |||
|
|||
# To many warnings, suppress them all (disable in case of troubleshooting) | |||
export CPPFLAGS="-w" | |||
export CXXFLAGS="-w" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This disables optimizations. You should add back at least -O or -O2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall try both an see what happens :)
@@ -4,6 +4,17 @@ | |||
|
|||
Traveling Ruby is a project which supplies self-contained, "portable" Ruby binaries: Ruby binaries that can run on any Linux distribution and any macOS machine. It also has Windows support [(with some caveats)](#caveats). This allows Ruby app developers to bundle these binaries with their Ruby app, so that they can distribute a single package to end users, without needing end users to first install Ruby or gems. | |||
|
|||
_Note:_ - This is a fork, which currently supports the following platforms and versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can be removed.
ARCHITECTURES = ["x86_64"] | ||
CONCURRENCY = `./internal/cpucount`.to_i | ||
ARCHITECTURES = ["x86_64",'arm64'] | ||
CONCURRENCY = `./internal/cpucount`.to_i + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why +1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure! I pulled that change in from this fork
75afdff#diff-fe8ec1b2975d083dd6bc6eaf2fca4dba61a6466d1991a1cbaf3c9dd27b841941L5
I imagine the internal cpu count, is going to be the max, so adding one doesn't make sense, will remove and retest
@@ -41,7 +41,7 @@ ARCHITECTURES.each do |arch| | |||
|
|||
desc "Build the #{arch} Docker image" | |||
task "image:#{arch}" do | |||
sh "docker build --pull" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no --pull?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I think that was from testing, I wasn't certain it was using the built image and not pulling the published versioned I had created with the same tag, I added that back in, in the ci
branch
I'll add it back in
@@ -255,12 +260,13 @@ echo | |||
|
|||
if $SETUP_SOURCE; then | |||
header "Configuring..." | |||
# NOTE: the option --disable-install-doc is a conjunction of --disable-install-rdoc and --disable-install-capi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to 3.2.2's ./configure --help
, --disable-install-doc is still available.
But I agree that disable-install-rdoc and disable-install-capi can be removed. Just disable-install-doc is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fab thanks
@@ -35,13 +35,13 @@ SKIP_LIBFFI=false | |||
FORCE_LIBYAML=false | |||
SKIP_LIBYAML=false | |||
FORCE_SQLITE3=false | |||
SKIP_SQLITE3=false | |||
SKIP_SQLITE3=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 38, 42 and 44 appear to be debugging changes that shouldn't be committed. They should be set back to false.
# To many warnings, suppress them all (disable in case of troubleshooting) | ||
export CPPFLAGS="-w" | ||
export CXXFLAGS="-w" | ||
export CFLAGS="-w" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These flags disable optimizations. They should contain -O or -O2.
@@ -0,0 +1,6 @@ | |||
source 'https://rubygems.org' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to have multiple native extension versions since it'll be too much of a hassle to maintain. Better merge all these directories into one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I noted that it was running through each of these gemfiles in shared, so in the CI branch I just have a single one which is commited in the ci
branch and also contains the lockfile
https://github.com/YOU54F/traveling-ruby/blob/ci/shared/gemfiles/20230508/Gemfile.lock
i assume the build jobs should be publishing the built gems as separate artefacts too, I noted your previous uploads did but not sure of the reason
GH actions were performed against the https://github.com/YOU54F/traveling-ruby/tree/ci/.github/workflows I originally built this on an m1 mac for osx builds (rosetta for x86_64), ubuntu multipass for the linux builds. CircleCI runs which include arm64 linux ( these upload artefacts each build ) Cirrus CI run which covers arm64 osx and linux (as well as x86 linux / windows) https://cirrus-ci.com/build/4778944688291840 Thanks for taking a look, I haven't massively reviewed the other changes that I pulled in, but did test out the built binaries for pact-ruby-standalone as I am familiar with that functionality and can test out the end changes in for me what are realistic scenarios, thank you for taking a look! pact-foundation/pact-ruby-standalone#70 (comment) x86_64 binaries were tested in GitHub actions https://github.com/YOU54F/pact-ruby-standalone/actions/runs/4671665215 windows test run for example https://github.com/YOU54F/pact-ruby-standalone/actions/runs/4671665215/jobs/8272957852 Linux ARM64 run - https://cirrus-ci.com/task/4943559351074816 |
Added a draft PR which includes all the CI workflows albeit which include a move to ruby 3.2.2 but might be worthwhile taking a look at, at some point I've noted down some of the bits where I got stuck |
I think it's better to directly upgrade to Ruby 3.2 instead of first doing Ruby 3.1. This reduces maintenance burden. I also think it's not necessary to include CircleCI and Cirrus. We're not going to use them anyway. Having only Github Actions is enough. |
As for CI ARM support: rather than using self-hosted runners I wonder whether you could use qemu-static instead of emulate ARM on Github Action's x86 runners. It won't be fast, but that way we won't need self-hosted runners. |
Actually... have I understood properly that Cirrus is free for public repositories? 🤔 Then it might be worthwhile using Cirrus only. |
Yes! So easy to run locally too, and the machines are full-fat, the github actions runners are wheezy and the qemu builds for arm64 builds of linux, take an eternity or fail. self-hosted is a different matter. I actually use macpine, and run lxd containers, as ephermal github actions https://beringresearch.github.io/macpine/lxd_macpine/ Ways to use it https://github.com/stgraber/lxd-github-actions I also run parallels on my machine, where i setup a ephemeral windows on runner, because reasons |
So I'm a little confused about pull request 132 vs 133. If we're skipping Ruby 3.1, does this mean 132 can be closed and we can focus on 133? |
yeah that probably makes sense |
Hey, going to close this one off, will apply your changes over the weekend as requested from here, and can centralise the discussions. Thanks buddy, appreciate your eyes across this! |
Upgrade to Ruby 3.1.2 / Support ARM64 OSX/Linux
Hey Hey @FooBarWidget 👋🏾
We are the maintainers of Pact! You might remember us from https://www.joyfulbikeshedding.com/blog/2021-01-06-the-future-of-traveling-ruby.html
We have since moved onto using Rust to build and package most of core functionality, but still get a-lot of asks to update the build system we use for packaging our Pact Ruby implementation, and its various gems into a standalone package, with travelling ruby.
I took a look a few times, but felt out of my depth, but have been watching a few forks closely on an off over the years, and finally managed to pul some work together from others, and added some work myself.
Issues affecting our community
Write a description of changes made
Future changes
In branch CI main...YOU54F:traveling-ruby:ci
ucrt
and needmsvs
version of Ruby (Windows on ARM users)BONUS POINTS
Avoid breakage
👍🏾
Release for 3.1.2
https://github.com/YOU54F/traveling-ruby/releases/tag/rel-20230504
Travelling ruby 3.1.2
MacOS
arm64 built on m1 pro
x86 built on intel i9
using changes from main...cloudaware:traveling-ruby:osx-3.1.2
Linux
x86 built on intel i9 using ubuntu multipass vm (running on mac)
using changes from main...cloudaware:traveling-ruby:3.1.2
aarch built on m1 pro using ubuntu multipass vm (running on mac)
using changes in commit YOU54F@ff9c059
Windows
x86_64 built on ryzen 5900x/win11 pro
Hasn't been heavily tested, bar with pact-ruby-standalone https://github.com/YOU54F/pact-ruby-standalone/releases/tag/v2.2.0 - YMMV
Release for 3.2.2
https://github.com/YOU54F/traveling-ruby/releases/tag/rel-20230508
Travelling ruby 3.2.2
Changes tested and released against a fork of pact-ruby-standalone project
https://github.com/YOU54F/pact-ruby-standalone/releases
You can see the traveling ruby binaries being pulled in here
https://github.com/YOU54F/pact-ruby-standalone/blob/master/tasks/package.rake#L218
Packages tested cross platform in CI
https://github.com/YOU54F/pact-ruby-standalone/actions/runs/4671665215
and consumed in pact-js (a heavily used project which consumed the ruby standalone project)
https://github.com/YOU54F/pact-js-core/blob/pact-node-arm64/standalone/install.ts#L28-L29
It previously wasn't able to run on arm64 linux machines, and on darwin had to use rosetta, I've now tested that without Rosetta installed, our ruby executables are looking good so far.
I think I would still like to use the squashfs approach used in ruby-packer, I did tickle both their source codes a little bit, but the forks were more progressed against yours, and it has been tried and tested with our applications for years.