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

JITSupport use breaks tests under non-CRuby #2

Closed
headius opened this issue Sep 29, 2021 · 4 comments
Closed

JITSupport use breaks tests under non-CRuby #2

headius opened this issue Sep 29, 2021 · 4 comments

Comments

@headius
Copy link
Contributor

headius commented Sep 29, 2021

In 590e09b @k0kubun added logic to clear JIT logs during the popen2 test using JITSupport.

This breaks the test on implementations that do not have the same JIT flags and RubyVM support as CRuby.

If this logic is still necessary for the test to run on CRuby with JIT enabled, it should be masked to only run on CRuby. If it is no longer needed it should be reverted.

The rest of the jit_support.rb file does not seem to be used and was only ported over because it's used by the full test suite in CRuby (ac2e7bd by @hsbt).

I have some changes coming to let the open3 tests run on JRuby and include our Windows support files (JRuby only launches subprocesses through Java APIs on Windows, so different logic is required) and can also include this fix.

@k0kubun
Copy link
Member

k0kubun commented Sep 29, 2021

Please feel free to branch based on defined?(RubyVM::JIT) in remove_mjit_logs or its caller. Thank you.

@headius
Copy link
Contributor Author

headius commented Sep 29, 2021

@k0kubun Will that get wiped out if someone updates jit_support.rb from CRuby again? I do not know the CRuby stdlib maintenance workflow, but such things have happened in the past.

@k0kubun
Copy link
Member

k0kubun commented Sep 29, 2021

I have never sync-ed test/lib from ruby/ruby to other repositories. So it's up to hsbt. But I suppose you can also just update the same file in ruby/ruby if you have the concern.

@headius
Copy link
Contributor Author

headius commented Sep 29, 2021

I will wait to hear from @hsbt before changing the logic in #3. Thanks!

@headius headius mentioned this issue Sep 29, 2021
@hsbt hsbt closed this as completed in 6b7ede6 Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants