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

Fix JRuby build #1401

Merged
merged 6 commits into from Dec 3, 2018
Merged

Fix JRuby build #1401

merged 6 commits into from Dec 3, 2018

Conversation

mvz
Copy link
Collaborator

@mvz mvz commented Aug 6, 2018

Fixes the JRuby build and makes it required.

@mvz
Copy link
Collaborator Author

mvz commented Aug 6, 2018

Still to do: SimpleCov.

@mvz
Copy link
Collaborator Author

mvz commented Dec 1, 2018

Dir.mktmpdir(prefix) seems broken different on JRuby 😱.

@mvz
Copy link
Collaborator Author

mvz commented Dec 1, 2018

Actually, seems like we're using undefined behavior by calling it as Dir.mktmpdir('/tmp'). The first argument is supposed to be a prefix of the created directory name, so foo would result in /tmp/foosomething. The second argument would be where you put the directory, so Dir.mktmpdir(nil, '/foo') would result in /foo/dsomething.

What we're doing creates /tmp/tmpsomething on MRI, and /tmpsomething on JRuby. Neither behavior is documented.

This clears up the spec by starting from an empty environment, so
the expectation tests the requirement more directly.
JRuby does not perform automatic conversion from RuntimeError to String.
There are two reasons for this:

- Coverage is not collected at all unless JRUBY_OPTS="--debug" is set,
  which slows down tests massively.
- Coverage percentage differs from the result on CRuby, making this
  check unreliable.
Java and hence JRuby always emits a message about this environment
variable, causing problems in output expectations.
The use of a first argument with a leading slash is undefined, breaks on
JRuby, and wasn't doing anything useful on MRI either. We can just use
the argument-less version.
@troessner
Copy link
Owner

Nice clean up. I'm happy to merge this as it is. Do you want to keep the current number of commits?

@mvz
Copy link
Collaborator Author

mvz commented Dec 3, 2018

Do you want to keep the current number of commits?

Yes, they carry some nice comments on the changes.

@troessner troessner merged commit 3dbf7d7 into master Dec 3, 2018
@troessner troessner deleted the fix-jruby-build branch December 3, 2018 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants