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

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
2 participants
@headius
Member

headius commented May 25, 2016

See #3907.

@headius headius added this to the JRuby 9.1.2.0 milestone May 25, 2016

Show outdated Hide outdated core/src/main/java/org/jruby/util/OSEnvironment.java
@@ -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") ) {

This comment has been minimized.

@ahorek

ahorek May 25, 2016

Contributor

if ( key.equalsIgnoreCase("PATH") ) {

@ahorek

ahorek May 25, 2016

Contributor

if ( key.equalsIgnoreCase("PATH") ) {

This comment has been minimized.

@headius

headius May 25, 2016

Member

Mmm, I suppose this would be in line with environment vars being case-insensitive on Windows, wouldn't it.

@headius

headius May 25, 2016

Member

Mmm, I suppose this would be in line with environment vars being case-insensitive on Windows, wouldn't it.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 25, 2016

Member

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.

Member

headius commented May 25, 2016

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.

headius added some commits May 25, 2016

Temporarily pass env values through bytes for locale encoding.
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.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 25, 2016

Member

Merged to master in 221ba4f. Merging to 9.1.2.0 branch now.

Member

headius commented May 25, 2016

Merged to master in 221ba4f. Merging to 9.1.2.0 branch now.

@headius headius closed this May 25, 2016

@headius headius deleted the headius:filesystem_encoding_for_path branch May 26, 2016

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