Skip to content

Test with TruffleRuby in CI - #402

Merged
hsbt merged 2 commits into
ruby:masterfrom
eregon:patch-1
Mar 19, 2020
Merged

hsbt merged 2 commits into
ruby:masterfrom
eregon:patch-1

Conversation

@eregon

@eregon eregon commented Jan 16, 2020 •

Copy link
Copy Markdown
Member

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: truffleruby/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

eregon commented Jan 16, 2020

Copy link
Copy Markdown
Member Author

@hsbt Could you review and merge this PR?

@eregon
eregon requested a review from flori January 16, 2020 06:19
@eregon

eregon commented Jan 25, 2020

Copy link
Copy Markdown
Member Author

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

@hsbt

hsbt commented Jan 25, 2020

Copy link
Copy Markdown
Member

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

At first, we should decide before merging this.

@eregon

eregon commented Jan 25, 2020

Copy link
Copy Markdown
Member Author

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

eregon commented Feb 9, 2020

Copy link
Copy Markdown
Member Author

@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

nurse commented Feb 11, 2020

Copy link
Copy Markdown
Member

In general who can decide this is flori.

@eregon

eregon commented Feb 15, 2020 •

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
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

eregon commented Mar 18, 2020

Copy link
Copy Markdown
Member Author

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

@headius

headius commented Mar 18, 2020

Copy link
Copy Markdown
Contributor

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

@hsbt

hsbt commented Mar 19, 2020

Copy link
Copy Markdown
Member

@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

eregon commented Mar 19, 2020

Copy link
Copy Markdown
Member Author

@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

hsbt commented Mar 19, 2020

Copy link
Copy Markdown
Member

@eregon Thanks!

@eregon

eregon commented Mar 25, 2020

Copy link
Copy Markdown
Member Author

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