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

SI-4615 Adds proper handling of -D and -J options Windows #1957

Closed
wants to merge 1 commit into from

Conversation

JamesIry
Copy link
Contributor

On Windows, -D and -J parameters weren't being added to JAVA_OPTS.

I've manually verified this patch on a Windows 7 VM, but we don't seem to have a way to run an automated test of the scala or scalac scripts.

review @adriaanm

On Windows, -D and -J parameters weren't being added to JAVA_OPTS.
@adriaanm
Copy link
Contributor

As discussed in person, I think not having a test for this is fine for now.
I don't know the first thing about windows scripts, so maybe @szeiger can comment?

Contributed by @huynhjl -- I think we need a CLA for this. Have you signed one already?

@scala-jenkins
Copy link

Started jenkins job JenkinsJob(pr-rangepos) at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1473/

@adriaanm
Copy link
Contributor

jenkins job pr-scala-testsuite-linux-opt: successful -- manually verified

@adriaanm
Copy link
Contributor

jenkins job pr-rangepos: successful -- manually verified

@huynhjl
Copy link

huynhjl commented Jan 24, 2013

More than a year and half after having wrote the patch, I have second thoughts on whether my fix is the right approach. I essentially wrote a tokenizer for the command line in BAT language and this is probably going to be fragile. I wonder if there are more reliable approaches by putting the -D arguments into a variable, instead.

@szeiger
Copy link
Contributor

szeiger commented Jan 24, 2013

I don't have much experience with the high wizardry of advanced cmd.exe scripting (and thanks to MSys I don't have to -- unless I'm using a Scala .sh script that doesn't work there) but I distilled a test.bat that logs the command line from this template and tried a few things on Windows 7 x64. My observations:

  • While this parses the -D and -J options, they also get passed as arguments to the class that is run in the end (and I expect that trying to fix this would make the other problems much worse)
  • Quoted -J options are silently dropped
  • Single-quoted -D options are silently dropped
  • Double-quoted -D options are either silently dropped or result in an error

@retronym
Copy link
Member

I just went over to check jruby.bat, and found it to be pretty small, as it just forwards to an .exe.

Maintaining JRuby.BAT was, well, to put it mildly, unpleasant. We had
tens of bugs due to BAT limitations, we had weird behaviors depending
on the version of Windows, we had a bunch of regressions.

See http://jira.codehaus.org/browse/JRUBY-4100 for more details.

https://github.com/vvs/jruby-launcher

@JamesIry
Copy link
Contributor Author

So is the consensus to close this PR? Or do we take it, flaws and all, as at least some improvement in the Windows situation until something better can be done?

@huynhjl
Copy link

huynhjl commented Jan 26, 2013

I'd vote to close this PR. Has the jruby.exe been working pretty well? May be it is the way to go. Quoting from http://jira.codehaus.org/browse/JRUBY-4100:

jruby.bat maintenance is tricky, we always getting new and new problems with it, applying hacks on top of hacks on top of hacks, and at the end it is still doesn't fully work 100% reliably. Not to mention that any kind of improvement to BAT file most definitely introduces all kinds of weird regressions.

Applying my patch would definitely be piling a hack that does not fully work.

@JamesIry
Copy link
Contributor Author

Just so everybody knows what we would be getting into by going native https://github.com/jruby/jruby-launcher

@adriaanm
Copy link
Contributor

i don't think we should go native, but if the OP retracts this PR, I don't think we should merge it and create expectations that aren't going to be met...

@JamesIry
Copy link
Contributor Author

K, whacking

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.

6 participants