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 quoted paths #3879

Merged
merged 1 commit into from May 13, 2016

Conversation

Projects
None yet
3 participants
@ahorek
Contributor

ahorek commented May 12, 2016

fixes #3878

Pavel Rosický
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 13, 2016

Member

Well, you're on a roll here :-)

We have generally recommended that JRuby always be installed in a path without spaces, since there's other Ruby libraries that don't like it. But if we can improve some cases, I'm all for it.

Member

headius commented May 13, 2016

Well, you're on a roll here :-)

We have generally recommended that JRuby always be installed in a path without spaces, since there's other Ruby libraries that don't like it. But if we can improve some cases, I'm all for it.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 13, 2016

Member

This seems pretty safe, doesn't it @enebo? Need to do some verification.

I'm a little curious why a path is getting in here that has quotes though. Is this a Windows shell quirk of some kind?

Member

headius commented May 13, 2016

This seems pretty safe, doesn't it @enebo? Need to do some verification.

I'm a little curious why a path is getting in here that has quotes though. Is this a Windows shell quirk of some kind?

@enebo enebo added this to the JRuby 9.1.1.0 milestone May 13, 2016

@enebo enebo merged commit 5508a08 into jruby:master May 13, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo May 13, 2016

Member

Yeah we must be passing through the quotes as $0 perhaps. I do not see the harm in this though. On unix you can use " in file name but I highly doubt it is valid on windows. Even if it is that would be crazy to use.

Member

enebo commented May 13, 2016

Yeah we must be passing through the quotes as $0 perhaps. I do not see the harm in this though. On unix you can use " in file name but I highly doubt it is valid on windows. Even if it is that would be crazy to use.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 13, 2016

Member

I think I remember reading that " is one of the explicitly-forbidden characters in filenames/paths.

On UNIX, the shell would be removing the quotes for us when it parses out arguments.

I do feel like there's something else going on here that is the actual root cause.

Member

headius commented May 13, 2016

I think I remember reading that " is one of the explicitly-forbidden characters in filenames/paths.

On UNIX, the shell would be removing the quotes for us when it parses out arguments.

I do feel like there's something else going on here that is the actual root cause.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 13, 2016

Member

@ahorek Maybe you can do a little research to figure out how this quoted string is getting into the runtime?

Member

headius commented May 13, 2016

@ahorek Maybe you can do a little research to figure out how this quoted string is getting into the runtime?

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo May 13, 2016

Member

Windows CMD does not strip any quoting or process globs (we even have an open bug since we are not handling this stuff on windows). Perhaps we wrote something in launcher/.bat assuming windows would handle this for us?

Member

enebo commented May 13, 2016

Windows CMD does not strip any quoting or process globs (we even have an open bug since we are not handling this stuff on windows). Perhaps we wrote something in launcher/.bat assuming windows would handle this for us?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 16, 2016

Member

@enebo That would make some sense. Maybe all quoted paths passed into our Windows launchers are wrong?

Member

headius commented May 16, 2016

@enebo That would make some sense. Maybe all quoted paths passed into our Windows launchers are wrong?

@ahorek

This comment has been minimized.

Show comment
Hide comment
@ahorek

ahorek May 17, 2016

Contributor

@headius
it's hardcoded in rubygems
https://github.com/jruby/jruby/blob/master/lib/ruby/stdlib/rubygems.rb#L854
https://github.com/rubygems/rubygems/blob/master/History.txt#L2558
according to the history Gem.ruby path is escaped because there were some issues with extensions compilation on windows

Contributor

ahorek commented May 17, 2016

@headius
it's hardcoded in rubygems
https://github.com/jruby/jruby/blob/master/lib/ruby/stdlib/rubygems.rb#L854
https://github.com/rubygems/rubygems/blob/master/History.txt#L2558
according to the history Gem.ruby path is escaped because there were some issues with extensions compilation on windows

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 18, 2016

Member

@ahorek Ahh that is interesting. So perhaps this quote-stripping is something we actually do need to do.

We should look at MRI's logic for process-launching on Windows and see if they strip this stuff too.

Member

headius commented May 18, 2016

@ahorek Ahh that is interesting. So perhaps this quote-stripping is something we actually do need to do.

We should look at MRI's logic for process-launching on Windows and see if they strip this stuff too.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 18, 2016

Member

I did not see any logic at a glance, but there's a lot of win32-specific process stuff.

Member

headius commented May 18, 2016

I did not see any logic at a glance, but there's a lot of win32-specific process stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment