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

File path canonicalization still broken inside jars on Windows #4692

Closed
presidentbeef opened this issue Jun 28, 2017 · 11 comments
Closed

File path canonicalization still broken inside jars on Windows #4692

presidentbeef opened this issue Jun 28, 2017 · 11 comments
Labels

Comments

@presidentbeef
Copy link

@presidentbeef presidentbeef commented Jun 28, 2017

Despite #4647 and earlier patches, this issue persists.

As previously, please see https://github.com/presidentbeef/jruby-realpath-error and the simple reproduction steps therein.

I hate to keep mentioning this, but due to this issue we are still stuck releasing on JRuby 1.7.

Expected Behavior

On Linux with JRuby 9.1.11.0:

$ java -jar myapp.jar
"uri:classloader:/lib/test.rb"
"classpath:/jar-bootstrap.rb"

Actual Behavior

On Windows with JRuby 9.1.11.0:

> java -jar myapp.jar
LoadError: no such file to load -- classpath:c:/lib/test
           require at org/jruby/RubyKernel.java:961
  require_relative at uri:classloader:/jruby/kernel/kernel.rb:13
            <main> at classpath:/jar-bootstrap.rb:1
@presidentbeef
Copy link
Author

@presidentbeef presidentbeef commented Jun 28, 2017

Should probably tag @kares

@stergiom
Copy link

@stergiom stergiom commented Jun 28, 2017

@presidentbeef, have you tried with 9.1.12.0? This worked for me.

@presidentbeef
Copy link
Author

@presidentbeef presidentbeef commented Jun 28, 2017

9.1.12.0 has the same behavior.

@enebo enebo added this to the JRuby 9.1.13.0 milestone Aug 16, 2017
@enebo enebo added the regression label Aug 16, 2017
@enebo
Copy link
Member

@enebo enebo commented Aug 30, 2017

@presidentbeef can you try latest on jruby-9.1.13 branch? @kares added some logic which should work a week or two ago and it did not get associated with this issue.

@enebo
Copy link
Member

@enebo enebo commented Sep 6, 2017

Ah I knew I had downloaded your test repo but now running it I can see require_relative is broken still although I think @kares patch did fix a good chunk of the issues around the fakepath part of it all. So here is a smaller repro:

relative_arg = "lib/test"
dir = "classpath:/"
p File.expand_path(relative_arg, dir)

This prints out

"classpath:C:/lib/test"

The C: should not be there...

@presidentbeef
Copy link
Author

@presidentbeef presidentbeef commented Sep 6, 2017

Thanks for checking @enebo.

@enebo
Copy link
Member

@enebo enebo commented Sep 6, 2017

Ok fixed the letter and now I see this:

"uri:classloader:\\lib\\test.rb"
"classpath:\\jar-bootstrap.rb"

GRRRRRRR, :)

@enebo
Copy link
Member

@enebo enebo commented Sep 6, 2017

Yay...so FILE is correct but File.realpath is swapping the / to \. So a second weird behavior.

@presidentbeef Is the reason for doing this to potentially elide stuff like '..' or '.'. I don't think classpath uris can actually have softlinks. With that said it no classpath: uri should ever get converted to back slashes. So it should just get fixed.

@presidentbeef
Copy link
Author

@presidentbeef presidentbeef commented Sep 6, 2017

Our app does have a lot of uses of require_relative "../../ but it would be nice for it to "just work" inside a jar.

enebo added a commit that referenced this issue Sep 6, 2017
…indows

This fixed two things in expandPathInternal which displayed differences in
File.expand_path, and File.realpath.  The fixes themselves are pretty gorey
but since they are limited to classpath uris and only on windows I see no
risk.  We really really need to rewrite expandPathInternal...
@enebo
Copy link
Member

@enebo enebo commented Sep 6, 2017

@presidentbeef yeah I think you should not need to be concerned with whether you are calling with real filesystem paths or a classpath uri. I just wondered if you were using it for '..'. I am pushing a fix here. I would love it if you could try your product on jruby-9.1 branch and see if it is golden now?

@enebo enebo closed this in c588a18 Sep 6, 2017
@presidentbeef
Copy link
Author

@presidentbeef presidentbeef commented Sep 9, 2017

9.1.13.0 works!! Thank you @enebo 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.