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

Add Truffleruby support #1611

Closed
wants to merge 4 commits into from
Closed

Add Truffleruby support #1611

wants to merge 4 commits into from

Conversation

gogainda
Copy link
Contributor

No description provided.

@quine-bot
Copy link

quine-bot bot commented Jan 29, 2022

👋 Figuring out if a PR is useful is hard, hopefully this will help.

  • @gogainda has been on GitHub since 2010 and in that time has had 107 public PRs merged
  • They haven't contributed to this repo before
  • Here's a good example of their work: fast-truffleruby (:dash: Writing Fast Ruby :heart_eyes: -- Collect Common Ruby idioms.)
  • From looking at their profile, they seem to be good with Ruby and Yacc.

Their most recently public accepted PR is: digital-fabric/extralite#5

@@ -22,6 +22,7 @@ module Formatter
@backtrace_filters << RbConfig::CONFIG['rubylibdir'] if RbConfig::CONFIG['rubylibdir']

@backtrace_filters << 'org/jruby/' if ::Cucumber::JRUBY
@backtrace_filters << '<internal:core>' if RUBY_ENGINE == "truffleruby"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eregon do you think it's safe change? It does the job (all internal errors are filtered out and tests are passing), but later on I noticed that some TypeError may dissapear as well, for example :

<internal:core> core/marshal.rb:51:in `__marshal__': singleton class can't be dumped (TypeError)
	from <internal:core> core/marshal.rb:1006:in `serialize'
	from <internal:core> core/marshal.rb:1309:in `dump'
	from -e:1:in `<main>'

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please fix the rubocop error (use single quotes), and add truffleruby to the GitHub Actions build matrix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link

Choose a reason for hiding this comment

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

It should be any path starting with <internal:, that's also used by CRuby:

$ ruby -ve 'p method(:then).source_location'
ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-linux]
["<internal:kernel>", 120]

Since it's converted to a Regexp later, then '^<internal:' is best.

@luke-hill
Copy link
Contributor

@gogainda it seems as though the existing CI fails some tests. I haven't got much time yet to check, but can you see if these are ones that should be passing or if this is a truffleruby nuance.

@gogainda
Copy link
Contributor Author

checking, tests have passed but the process failed with:

Error: Process completed with exit code 137.

@aslakhellesoy
Copy link
Contributor

checking, tests have passed but the process failed with:

Error: Process completed with exit code 137.

Yes, looks like it: https://github.com/cucumber/cucumber-ruby/runs/4999431897?check_suite_focus=true#step:3:459

I think we need to get to the bottom of what's causing that because we merge this. We need to be confident that all tests pass on truffleruby.

@gogainda
Copy link
Contributor Author

Will close it for a while until I've found a root cause

@gogainda gogainda closed this Jan 31, 2022
@gogainda
Copy link
Contributor Author

gogainda commented Feb 1, 2022

@aslakhellesoy I've added additional workflow for rubocop job.

Since output for every version is the same , we want to run it only once and leave ruby matrix only for testing

@gogainda
Copy link
Contributor Author

gogainda commented Feb 1, 2022

created another PR for it #1612

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.

4 participants