Dir.glob with classpath: resource as a base is broken
#6421
Comments
|
I wasn't able to find a clear definition of the difference between the For context, we use it as an element in our configured load path for JRuby: https://github.com/puppetlabs/puppetserver/blob/74af3b155550c843b5705bbad263cb82940b0cfb/src/clj/puppetlabs/services/jruby/jruby_puppet_core.clj#L23-L35 |
|
Thanks for the report! My memory is a bit fuzzy on the difference, but I believe "classpath" was the older URL format and we generally prefer "uri:classloader" now, but I agree they essentially mean the same thing and should behave the same. |
|
Reproduced with a self-contained example: Note that this isn't just an error, it's a Java IllegalArgumentException error, so clearly violating our goal of hiding the Java bits of JRuby. |
|
Note also removing the leading "/" does not solve the "classpath" case, though it works both ways with "uri:classloader". |
|
This appears to be a symptom of inconsistent normalization. We have code in JRubyFile to normalize "classpath:" to "uri:classloader:", but the glob logic doesn't use the normalized path other than to check the existence of the resource. So in the case here, it normalizes to "uri:classloader:/jni", sees that that resource exists, and then continues on using "classpath:/jni" which is not understood by the rest of the globbing logic. So there's two fixes here that make sense independently or together:
A patch for the first part, which is sufficient to fix this issue: diff --git a/core/src/main/java/org/jruby/RubyDir.java b/core/src/main/java/org/jruby/RubyDir.java
index 4eb1385894..8916e70a05 100644
--- a/core/src/main/java/org/jruby/RubyDir.java
+++ b/core/src/main/java/org/jruby/RubyDir.java
@@ -300,11 +300,18 @@ public class RubyDir extends RubyObject implements Closeable {
String base = globOptions(context, args, flags);
List<ByteList> dirs;
- if (base != null && !base.isEmpty() && !(JRubyFile.createResource(context, base).exists())){
+ if (base == null || base.isEmpty()) {
+ base = runtime.getCurrentDirectory();
+ }
+
+ // createResource will normalize URLs (see GH-6421)
+ FileResource resource = JRubyFile.createResource(context, base);
+
+ if (!(resource.exists())){
dirs = new ArrayList<ByteList>();
} else {
IRubyObject tmp = args[0].checkArrayType();
- String dir = base == null || base.isEmpty() ? runtime.getCurrentDirectory() : base;
+ String dir = resource.absolutePath();
if (tmp.isNil()) {
dirs = Dir.push_glob(runtime, dir, globArgumentAsByteList(context, args[0]), flags[0]);I'm on the fence about whether to fix the rest of the glob internals to support "classpath:". If such a fix were made, it would likely be simplest to normalize the URI in the same way, which then begs the question of why it's not normalized earlier in the call stack as in the above patch. The patch above is the lowest-impact fix that works, and does exactly that: normalizes the URI before proceeding into glob internals. |
The createResource call here normalizes a URI like "classpath:" to our preferred "uri:classloader:" but only uses that normalized version to check the existence of the resource in question. If the resource exists, it proceeds to use the un-normalized version in glob, and fails since "classpath:" is not supported by our glob internals. This patch uses the newly normalized path when calling glob internals, so "classpath:" never leaks into that code. Fixes jruby#6421
Not directly related to jruby#6421 but found along the way.
|
The patch and connected PR appear to cause some issues loading resources from jar files, so this needs another look. |
|
This is now affecting autoload-based systems like Zeitwork when used inside a jar context, such as on Android. |
Environment Information
Provide at least:
Other relevant info you may wish to add:
irbin a leiningen REPLExpected Behavior
Dir.globshould be able to take aclasspath:<filename>resource as a base, since it apparently supportsuri:classloader:<filename>.Actual Behavior
We observed the following, starting a REPL by running
lein irbin a checkout of a the puppetserver repo. Apologies that I'm not sure how to properly set up a classpath outside of this environment for a simpler repro:This worked on 9.2.8.0. When we upgraded to 9.2.11.0, it stopped working. We've switched to using
uri:classloaderfor now as a workaround, but since this is a regression, and from looking at the code,classpathstill seems to be generally supported, this seems like a bug and not an intentional change?I did some spelunking, and noticed the following oddity that might explain this, even if the fix isn't exactly in that part of the code. I think we want to hit this code path but don’t, because while it converts a
pathnamewithclasspathin it touri:classloaderhere, it doesn’t do the same forcwd, so instead we fall through to this which incorrectly treats it as a normal file. It's unclear to me if there's supposed to be a conversion forcwdtoo, or if this code expects that to already have been processed more before we get here. But hope that finding helps track this down.Let me know if you need any more information.
The text was updated successfully, but these errors were encountered: