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 -x command line option to skip header #5659

Merged
merged 1 commit into from Apr 9, 2019

Conversation

@kschoelhorn
Copy link
Contributor

@kschoelhorn kschoelhorn commented Mar 21, 2019

The -x option has been broken since 9e2d6dc, as parseShebangOptions is no longer run, which is responsible for calling setHasShebangLine. As this is no longer done, hasShebangLine always returns false, even if there is a valid shebang.

The fix is to remove the check at this location and move it into the findScript function, which already searches for the ruby shebang.

Like mentioned in #4536, there seems to be a test for this, but it is not run during CI (at least I couldn't find it). Also parseShebangOptions seems to be dead code, which I can also remove if you want.


boolean foundRubyShebang = false;
String currentLine;
while((currentLine = br.readLine()) != null) {
Copy link
Member

@olleolleolle olleolleolle Mar 26, 2019

Choose a reason for hiding this comment

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

One tiny thing: This while statement has different whitespace.

It needs a space after the while keyword to keep it the same.

Same goes for the if statements below.

(Thanks for working to imrpove JRuby!)

Loading

Copy link
Contributor Author

@kschoelhorn kschoelhorn Mar 26, 2019

Choose a reason for hiding this comment

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

Oops, you are totally right, fixed!

Loading

The -x option has been broken since [1], as parseShebangOptions is no
longer run, which is responsible for calling setHasShebangLine. As this
is no longer done, hasShebangLine always returns false, even if there is
a valid shebang.

The fix is to remove the check at this location and move it into the
findScript function, which already searches for the ruby shebang.

[1]: 9e2d6dc

Fix: jruby#4536
@kschoelhorn kschoelhorn force-pushed the fix-dash-x-option branch from 9dcf510 to 4081dd9 Mar 26, 2019
@headius
Copy link
Member

@headius headius commented Apr 9, 2019

Failures looked like some intermittent issues we've had on all branches, so I think we can go forward with this. Did not make 9.2.7, but it's in for 9.2.8!

Loading

@headius headius added this to the JRuby 9.2.8.0 milestone Apr 9, 2019
@headius headius merged commit 1474f2b into jruby:master Apr 9, 2019
1 of 2 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants