[JEP-206] Always use UTF-8 for per-step log files #56
Conversation
| if(charset==null) return Charset.defaultCharset(); // just being defensive | ||
| return Charset.forName(charset); | ||
| if (charset == null) { // new style | ||
| return StandardCharsets.UTF_8; |
There was a problem hiding this comment.
Is this actually safe to do? Wouldn't some legacy builds potentially be in other Charsets?
Seems like we should default the same way we did -- but always initialize with UTF-8, or am I missing something here? (Granted I'm pretty out of it at the moment, so that's possible.)
Some of this might be why windows breaks.
There was a problem hiding this comment.
Yes historical builds would typically be in some other charset—which is why their charset field would be set, and we would honor that setting. It is only new builds which omit the field and thus use UTF-8 regardless of the master’s system default encoding.
svanoort
left a comment
svanoort
left a comment
There was a problem hiding this comment.
Windows test failure -- and I'm not convinced this is safe for loading previous builds' logs if the encoding is nonstandard somehow.
Test can be strengthened only after jenkinsci/workflow-job-plugin#89 is released, so we do not get into a cyclic dependency (or by rewriting this aspect of the test to save content into a file and read it from test code).
|
The Windows test failure is a bit misleading. It happened to pass before because the character being expected, |
svanoort
left a comment
svanoort
left a comment
There was a problem hiding this comment.
Looks safe AFAICT -- but pending a quick smoketest of the full stack.
Well, more likely CP-1252 actually, but same applies. |
To be precise: jenkinsci/workflow-job-plugin#89 (and this, its dependency), plus jenkinsci/workflow-durable-task-step-plugin#64 (and jenkinsci/durable-task-plugin#61, its dependency). As a user, since the plugin manager deals with dependencies transparently, it means that you should update both Pipeline: Job and Pipeline: Nodes and Processes if you deal with funny characters. The fixes will apply to newly started builds (nothing should change for historical builds). |
svanoort
left a comment
svanoort
left a comment
There was a problem hiding this comment.
Blocking until we have some validation of the end-to-end scenarios (pending).
jglick
changed the title
4 participants
JEP-206
Not exactly pulled out of #15, but in the same vein and complementary. One component of the solution only; as noted in jenkinsci/workflow-job-plugin#89, users will need to update multiple plugins to resolve issues.