Skip to content

(PUP-8226) Set JRuby KCode, source encoding, and external encoding#61

Merged
haus merged 2 commits intopuppetlabs:masterfrom
Magisus:kcode
Jan 24, 2018
Merged

(PUP-8226) Set JRuby KCode, source encoding, and external encoding#61
haus merged 2 commits intopuppetlabs:masterfrom
Magisus:kcode

Conversation

@Magisus
Copy link
Contributor

@Magisus Magisus commented Jan 24, 2018

This commit adds a function that can configure the KCode, external
encoding, and source encoding of the JRuby scripting containers. We use
this function when initializing our JRuby instances to set these
encodings to UTF-8, to avoid parsing source files as US-ASCII, which can
cause issues with translation interpolation.

Note that this is only relevant in JRuby 1.7; in JRuby 9K, source files are parsed
as UTF-8 by default.

(let [encoding-string (str (.getEncoding kcode))]
;; RubyInstanceConfig exposes the encoding setters directly, while the
;; ScriptingContainer does not.
(if (instance? RubyInstanceConfig jruby)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it is worth being explicit here and using cond instead of if so that we can test for both RubyInstanceConfig and ScriptingContainer?

@haus
Copy link
Contributor

haus commented Jan 24, 2018

We talked in person about this, but I think it's worth breaking the function up into one that expects a RubyInstanceConfig and one that gets a RubyInstanceConfig as all of the calls are the same once a RubyInstanceConfig exists.

@Magisus
Copy link
Contributor Author

Magisus commented Jan 24, 2018

Updated.

@Magisus
Copy link
Contributor Author

Magisus commented Jan 24, 2018

Note, this proved really hard to test at the unit level, because the encoding bug only appears when parsing Ruby source files to execute, which is difficult to do in the context of the configured scripting container.

Edit: Haus found a way to test this.

@haus
Copy link
Contributor

haus commented Jan 24, 2018

Nope. I totally didn't. My test passes, but doesn't fail without this change, so that's too bad.

This commit adds a function that can configure the KCode, external
encoding, and source encoding of the JRuby scripting containers. We use
this function when initializing our JRuby instances to set these
encodings to UTF-8, to avoid parsing source files as US-ASCII, which can
cause issues with translation interpolation.

Note that this is only relevant in JRuby 1.7; in JRuby 9K, source files are parsed
as UTF-8 by default.
(org.jruby.runtime.profile.builtin ProfileOutput)
(java.io File)))
(java.io File)
(org.jruby.util KCode)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: if we're changing this can we put the imports in alphabetical order?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

(.setKCode kcode)
(.setSourceEncoding encoding-string)
(.setExternalEncoding encoding-string)))
jruby)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doto returns its first argument so this is unnecessary

(if (instance? RubyInstanceConfig jruby)
(set-config-encoding kcode jruby)
(set-config-encoding kcode (.getRubyInstanceConfig (.getProvider jruby))))
jruby)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@haus haus merged commit 111d0fd into puppetlabs:master Jan 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants