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

Test the new integrated rb-sys #228

Merged
merged 13 commits into from Mar 9, 2023
Merged

Test the new integrated rb-sys #228

merged 13 commits into from Mar 9, 2023

Conversation

gjtorikian
Copy link
Owner

@gjtorikian gjtorikian force-pushed the test-new-integrated-rb-sys branch 2 times, most recently from e862163 to a7f7aa8 Compare February 5, 2023 18:46
@ianks
Copy link

ianks commented Feb 6, 2023

0.9.63 has been released which officially releases this!

@gjtorikian
Copy link
Owner Author

0.9.63 has been released which officially releases this!

Heck yeah, thank you!

@ianks
Copy link

ianks commented Feb 7, 2023

I need to add a better error message, but now the extension task expects the crate name: RbSys::ExtensionTask.new(CRATE_NAME_FROM_CARGO_TOML_HERE)

Looks like they already match in your code. So I'm wondering why cargo metadata isn't working, and a case I probably need to handle

@ianks
Copy link

ianks commented Feb 7, 2023

This diff should fix it:

diff --git i/commonmarker.gemspec w/commonmarker.gemspec
index a45619e..51b564b 100644
--- i/commonmarker.gemspec
+++ w/commonmarker.gemspec
@@ -17,7 +17,7 @@ Gem::Specification.new do |spec|
   # https://github.com/rubygems/rubygems/pull/5852#issuecomment-1231118509
   spec.required_rubygems_version = ">= 3.3.22"
 
-  spec.files = ["LICENSE.txt", "README.md", "Cargo.lock"]
+  spec.files = ["LICENSE.txt", "README.md", "Cargo.lock", "Cargo.toml"]
   spec.files += Dir.glob("lib/**/*.rb")
   spec.files += Dir.glob("ext/**/*.{rs,toml,lock,rb}")
   spec.bindir = "exe"

@gjtorikian
Copy link
Owner Author

This diff should fix it:

thanks!

@gjtorikian gjtorikian force-pushed the test-new-integrated-rb-sys branch 6 times, most recently from 29bcc48 to 3d14d7a Compare February 7, 2023 17:55
@gjtorikian gjtorikian force-pushed the test-new-integrated-rb-sys branch 3 times, most recently from 76656d4 to 3d0769c Compare February 26, 2023 16:20
Commonmarker currently has three different test suites related to the
gem build:

1. The compilation output is tested
2. The gem can be installed on another platform
3. A generic version of the gem can be installed on a platform with
sufficient Ruby/Rust tooling

Of these, only the first is the most relevant. 2 suggests an error in
the build pipeline, and 3 opens the door to supporting platforms that
are uncommon. In order to minimize support work
on my end, I'm going to nix the last two tests; as
well, I can abstract the first test into a GitHub Action for other
oxidizers to use.
@gjtorikian gjtorikian merged commit c5cf288 into main Mar 9, 2023
@gjtorikian gjtorikian deleted the test-new-integrated-rb-sys branch March 9, 2023 20:28
@ianks
Copy link

ianks commented Mar 9, 2023

BIG 🟥 diff, love it!

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