-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Use "filesystem" encoding for PATH as in MRI. Fixes #3907. #3923
Conversation
@@ -111,17 +111,22 @@ | |||
val = entry.getValue(); | |||
if ( ! (val instanceof String) ) continue; // Java devs can stuff non-string objects into env | |||
|
|||
putRubyKeyValuePair(runtime, rubyMap, key, (String) val, encoding); | |||
Encoding valueEncoding = keyEncoding; | |||
if ( key.equals("PATH") ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( key.equalsIgnoreCase("PATH") ) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, I suppose this would be in line with environment vars being case-insensitive on Windows, wouldn't it.
The failure was valid; my changes were a bit overreaching in trying to get encodings right. I've reverted the modified behavior for "locale" encoding and everything is green again. |
Fixes the rest of jruby#3907.
This fixes the one regression in my previous changes. Because MRI always creates new strings when you read from ENV, they just normalize to locale encoding for setenv, and then either use locale encoding or filesystem encoding for getenv. The equivalent for us here is to just let the setenv string decode to Java String and then use the default getBytes to match it up with the default Charset used for locale, rather than attempting to transcode the string directly.
46bce29
to
9d7295d
Compare
Merged to master in 221ba4f. Merging to 9.1.2.0 branch now. |
See #3907.