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 with TruffleRuby in CI #402

Merged
merged 2 commits into from
Mar 19, 2020
Merged

Test with TruffleRuby in CI #402

merged 2 commits into from
Mar 19, 2020

Conversation

eregon
Copy link
Member

@eregon eregon commented Jan 16, 2020

The latest release of JSON (2.3.0) unfortunately broke TruffleRuby 19.3.0, as it used rb_gc_register_mark_object which was not yet supported by TruffleRuby: oracle/truffleruby#1856
The TruffleRuby 19.3.1 release fixes it.

It would be useful to test TruffleRuby in CI to notice when it breaks, and if possible avoid breaking it.

I checked and it fails as expected when using a not-yet-implemented C API function.

@eregon eregon marked this pull request as ready for review January 16, 2020 06:15
@eregon
Copy link
Member Author

eregon commented Jan 16, 2020

@hsbt Could you review and merge this PR?

@eregon eregon requested a review from flori January 16, 2020 06:19
@eregon
Copy link
Member Author

eregon commented Jan 25, 2020

@nurse @hsbt @flori Ping, could you merge this?

@hsbt
Copy link
Member

hsbt commented Jan 25, 2020

I'm negative to merge this. Because we didn't support TruffleRuby officially yet.

At first, we should decide before merging this.

@eregon
Copy link
Member Author

eregon commented Jan 25, 2020

At first, we should decide before merging this.

Who can decide that?

JSON is part of the standard library and widely used, it is critical that it works on all major Ruby implementations, that is MRI, JRuby and TruffleRuby.

Supporting TruffleRuby currently requires 0 changes in the gem (unlike JRuby which needs a completely different extension). I think it doesn't make sense to support JRuby but not TruffleRuby in that context.

I want to clarify that merging this PR doesn't mean the new TravisCI TruffleRuby job must prevent merging changes.
But if it fails, it should be a conscious decision (i.e., not accidental) and of course it will be great if someone from TruffleRuby knows about it. I can take care of monitoring the CI status here for that.

@eregon
Copy link
Member Author

eregon commented Feb 9, 2020

@hsbt How can we progress on this PR? What should we wait for?

I'm not asking for official support of TruffleRuby in JSON, just adding it in CI.

FWIW, I'm happy to fix any issue coming with TruffleRuby in JSON.
We can just move truffleruby to allow_failures if it fails (obviously would be great if I know about that, but I can also add some automated monitoring on my side).

Given that, can we just merge this PR?

@nurse
Copy link
Member

nurse commented Feb 11, 2020

In general who can decide this is flori.

@eregon
Copy link
Member Author

eregon commented Feb 15, 2020

@flori Could you review and merge this PR?

It would be great to have other maintainers for such a critical part of the Ruby standard library.
Can we consider @hsbt @nurse and @headius as maintainers alongside you, which means they should be able to make all decisions needed to maintain this project? (including merging PRs, adding support for new implementations, publish releases, push the gem, etc).
From https://bugs.ruby-lang.org/issues/15605 and many reports before, it's clearly not enough to have a single not-so-active maintainer.
I don't think it's sensible to require the project creator for such a small PR. Therefore I'd like to address the general situation here.

Maybe transferring the project to https://github.com/ruby/json would help to clarify this is a Ruby community-maintained gem. Most other gems in the standard library are under https://github.com/ruby, so I think it would be consistent.

@flori What do you think of that? Of course, we can move this discussion to an issue or somewhere else if you prefer.

EDIT: moved to #412

@MSP-Greg
Copy link
Contributor

See PR #406 which add Actions CI for all platforms. TruffleRuby is passing on both Ubuntu & macOS, along with all MRI Rubies 2.2 thru head, on all three platforms...

@eregon
Copy link
Member Author

eregon commented Mar 18, 2020

@flori This PR has been open for 2 months, could you please take a look at it?

@headius
Copy link
Contributor

headius commented Mar 18, 2020

I would merge if I had push rights, but I do not.

@hsbt
Copy link
Member

hsbt commented Mar 19, 2020

@eregon Can you also add the truffleruby to allow_failures? After that, I'm going to merge this.

We can just move truffleruby to allow_failures if it fails (obviously would be great if I know about that, but I can also add some automated monitoring on my side).

It's your convenience, not maintainer's one. I was always tired to maintain CI matrix for no responsible platform.

@eregon
Copy link
Member Author

eregon commented Mar 19, 2020

@hsbt Done.

I think it's not much effort for the potential PR breaking TruffleRuby to add it in allow_failures, but I understand it might be annoying e.g., when synchronizing commits from ruby/ruby.
I'll find a way to monitor the result of TravisCI on this repo so I don't miss it if it breaks.

@hsbt hsbt merged commit da50acc into ruby:master Mar 19, 2020
@hsbt
Copy link
Member

hsbt commented Mar 19, 2020

@eregon Thanks!

@eregon
Copy link
Member Author

eregon commented Mar 25, 2020

Thanks for merging!

@flori Could you reply to #402 (comment) once you see this?

This was referenced Mar 14, 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