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

Migrate to magnus/rb_sys #194

Merged
merged 2 commits into from
Nov 3, 2022
Merged

Migrate to magnus/rb_sys #194

merged 2 commits into from
Nov 3, 2022

Conversation

gjtorikian
Copy link
Owner

The previous attempt to create a gem based on a Rust lib worked, but I could not for the life of me figure out MingW build errors in the FFI.

After searching the Net for some help I came across rb-sys and magnus; the former makes packaging Rust libs much easier than the mini_portile2 method I was using before, and the latter enabled writing the glue binding code to be written in Rust, not C. Beautiful, incredible.

@gjtorikian gjtorikian force-pushed the call-forth-the-dark-lord branch 21 times, most recently from 36a19d1 to f863f97 Compare October 18, 2022 21:27
Copy link

@ianks ianks left a comment

Choose a reason for hiding this comment

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

Awesome stuff, super excited to see this happening. Let me know if you have any questions along the way. Happy to help!

.github/workflows/gem-build-and-install.yml Outdated Show resolved Hide resolved
@@ -0,0 +1,134 @@
# frozen_string_literal: true

CrossRuby = Struct.new(:version, :platform) do
Copy link

Choose a reason for hiding this comment

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

FWIW, I’ve tried to package some of this data in rb-sys, and would love to add support for anything else that’s useful if it’s missing anything!

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hell yeah, thanks!

Copy link
Owner Author

Choose a reason for hiding this comment

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

That link goes to the wrong project, I think--is the rb-sys logic in RbSys::ToolchainInfo ? I'm not seeing an easy way to just do like if windows? using pure rb-sys.

Copy link

Choose a reason for hiding this comment

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

Good point, seems useful to have. I'd be happy to add that.

# Ruby doesn't know that it's on a musl-based platform. `ldd` is the
# a more reliable way to detect musl.
# See https://github.com/skylightio/skylight-ruby/issues/92
if ENV["SKYLIGHT_MUSL"] || %x(ldd --version 2>&1).include?("musl")
Copy link

Choose a reason for hiding this comment

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

We just merged in support for musl builds in rb-sys and could help out trying to iron it all out if you join the Slack

Copy link
Owner Author

Choose a reason for hiding this comment

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

Heck yeah, will join for sure. I’ve been hacking together Rust in Ruby for so long, I’m excited to see tooling and a community behind it!

@gjtorikian
Copy link
Owner Author

gjtorikian commented Oct 19, 2022

Awesome stuff, super excited to see this happening. Let me know if you have any questions along the way. Happy to help!

Thanks! The rewrite to use Rust “glue” has happened and was tremendously easy. My biggest fight right now is figuring out how to automate the compilation and packaging for all the operating systems I want to support. This has raised a number of problems, most of which are GitHub Actions runner related:

  • “latest” doesn’t actually mean the latest OS; for example, runs-on: macos-latest means macOS 11, not 12.
  • The GitHub runners don’t support arm64
  • The GitHub windows runners can’t compile the gem—although I’ll have to recheck this, as the windows CI tests DO run
  • The aarch64 builds don’t work (due to Errors on arm-linux matsadler/magnus#17 ?)

I just want x86_64 and arm64 builds for Linux, Windows, and macOS. I got it working locally but I’m really struggling with the runners. 😞

@ianks
Copy link

ianks commented Oct 19, 2022

aarch64-linux builds to indeed work with magnus, but arm-linux (32 bit) do not yet.

I just want x86_64 and arm64 builds for Linux, Windows, and macOS. I got it working locally but I’m really struggling with the runners. 😞

The cross-gem-action should support all of these. Here is a good working reference. These images are all based on rake-compiler-dock. I would recommend using these images since they have all of the right libc versions and patches in place, whereas the github runner images may have portability issues.

@gjtorikian gjtorikian force-pushed the call-forth-the-dark-lord branch 3 times, most recently from 3d4eacd to 17c66f7 Compare November 2, 2022 19:24
@gjtorikian gjtorikian force-pushed the call-forth-the-dark-lord branch 3 times, most recently from e3de5b9 to 97fa6ca Compare November 2, 2022 20:36
- uses: oxidize-rb/cross-gem-action@main
with:
platform: ${{ matrix.platform }}
env: |
Copy link

Choose a reason for hiding this comment

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

Should be good to support 2.7+

Suggested change
env: |
ruby-versions: '3.1, 3.0, 2.7'

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, but I don't wanna 😝

The real reason is that I don't want to maintain two Windows architectures for the next few years. Ruby 3.1 changed the toolchain string and I don't want to carry the cognitive load of "if < 3.1, do X." People will just have to deal. 🤷🏻‍♂️

run: |
./script/test-gem-build gems ${{matrix.platform}}

- uses: actions/upload-artifact@v3
Copy link

Choose a reason for hiding this comment

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

cross-gem-action does perform an upload of the artifact, so this may not be necessary?


fn iterate_extension_options(comrak_options: &mut ComrakOptions, options_hash: RHash) {
options_hash.foreach(|key: Symbol, value: Value| {
if key.name().unwrap() == "strikethrough" {
Copy link

Choose a reason for hiding this comment

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

nit: this may read better as a match, and avoid some allocations:

match key.name() {
  Ok("strikethrough") => ...,
  Ok("tagfilter") => ...,
}

@gjtorikian gjtorikian force-pushed the call-forth-the-dark-lord branch 7 times, most recently from 42d70f0 to 04d5436 Compare November 3, 2022 18:29
@gjtorikian gjtorikian merged commit 1bf0470 into release-v1 Nov 3, 2022
@gjtorikian gjtorikian deleted the call-forth-the-dark-lord branch November 3, 2022 18:44
@gjtorikian gjtorikian mentioned this pull request Nov 3, 2022
1 task
@gjtorikian gjtorikian mentioned this pull request Nov 11, 2022
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

2 participants