Skip to content

[JEP-206] Always use UTF-8 for per-step log files #56

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.

Sign up for GitHub

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

Merged
svanoort merged 5 commits into from
Aug 7, 2018
Merged

[JEP-206] Always use UTF-8 for per-step log files #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f557afe
[JENKINS-31096] Always use UTF-8 for per-step log files.
jglick Feb 9, 2018
1c05eec
Test failure on systems using non-UTF-8 default encodings.
jglick Feb 12, 2018
918f595
Merge branch 'master' into UTF-8-JENKINS-31096
jglick Jun 7, 2018
959c270
Merge branch 'master' into UTF-8-JENKINS-31096
jglick Aug 6, 2018
8b4964a
Updated parent and reincrementalified.
jglick Aug 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .mvn/extensions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@
<extension>
<groupId>io.jenkins.tools.incrementals</groupId>
<artifactId>git-changelist-maven-extension</artifactId>
<version>1.0-beta-3</version>
<version>1.0-beta-4</version>
</extension>
</extensions>
6 changes: 3 additions & 3 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>3.12</version>
<version>3.19</version>
<relativePath />
</parent>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-support</artifactId>
<version>2.20-SNAPSHOT</version>
<version>${revision}${changelist}</version>
<packaging>hpi</packaging>
<name>Pipeline: Supporting APIs</name>
<url>https://wiki.jenkins.io/display/JENKINS/Pipeline+Supporting+APIs+Plugin</url>
Expand Down Expand Up @@ -62,7 +62,7 @@
</pluginRepository>
</pluginRepositories>
<properties>
<revision>2.19</revision>
<revision>2.20</revision>
<changelist>-SNAPSHOT</changelist>
<jenkins.version>2.62</jenkins.version>
<java.level>8</java.level>
Expand Down
16 changes: 9 additions & 7 deletions src/main/java/org/jenkinsci/plugins/workflow/support/actions/LogActionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.io.PrintStream;
import java.io.UnsupportedEncodingException;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.concurrent.atomic.AtomicReference;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand Down Expand Up @@ -73,15 +74,14 @@ public class LogActionImpl extends LogAction implements FlowNodeAction, Persiste
public static @Nonnull TaskListener stream(final @Nonnull FlowNode node, @CheckForNull ConsoleLogFilter filter) throws IOException, InterruptedException {
LogActionImpl la = node.getAction(LogActionImpl.class);
if (la == null) {
// TODO: use UTF-8
la = new LogActionImpl(node, Charset.defaultCharset());
la = new LogActionImpl(node);
node.addAction(la);
}
OutputStream os = new FileOutputStream(la.getLogFile(), true);
if (filter != null) {
os = filter.decorateLogger((AbstractBuild) null, os);
}
final StreamTaskListener result = new StreamTaskListener(os);
final StreamTaskListener result = new StreamTaskListener(os, la.getCharset());
final AtomicReference<GraphListener> graphListener = new AtomicReference<>();
LOGGER.log(Level.FINE, "opened log for {0}", node.getDisplayFunctionName());
graphListener.set(new GraphListener.Synchronous() {
Expand All @@ -101,12 +101,11 @@ public class LogActionImpl extends LogAction implements FlowNodeAction, Persiste
private transient volatile File log;
private String charset;

private LogActionImpl(FlowNode parent, Charset charset) throws IOException {
private LogActionImpl(FlowNode parent) throws IOException {
if (!parent.isActive()) {
throw new IOException("cannot start writing logs to a finished node " + parent + " " + parent.getDisplayFunctionName() + " in " + parent.getExecution());
}
this.parent = parent;
this.charset = charset.name();
}

@Restricted(DoNotUse.class) // Jelly
Expand Down Expand Up @@ -162,8 +161,11 @@ public void onLoad(FlowNode parent) {
}

private Charset getCharset() {
if(charset==null) return Charset.defaultCharset(); // just being defensive
return Charset.forName(charset);
if (charset == null) { // new style
return StandardCharsets.UTF_8;
Copy link
Member

@svanoort svanoort Feb 9, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@jglick jglick Feb 12, 2018

Choose a reason for hiding this comment

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

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.

} else { // historical
return Charset.forName(charset);
}
}

@Override public String toString() {
Expand Down
5 changes: 4 additions & 1 deletion src/test/java/org/jenkinsci/plugins/workflow/support/steps/build/RunWrapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,10 @@ public class RunWrapperTest {
"echo \"currentBuild.projectName='${currentBuild.projectName}'\"\n" +
"echo \"currentBuild.fullProjectName='${currentBuild.fullProjectName}'\"\n", true));
WorkflowRun b = r.j.assertBuildStatusSuccess(p.scheduleBuild2(0).get());
r.j.assertLogContains("currentBuild.fullDisplayName='this-folder » this-job #1'", b);
{ // TODO mojibake until https://github.com/jenkinsci/workflow-job-plugin/pull/89 is released and can be consumed as a test dependency; expect this-folder » this-job #1
r.j.assertLogContains("currentBuild.fullDisplayName='this-folder", b);
r.j.assertLogContains("this-job #1'", b);
}
r.j.assertLogContains("currentBuild.projectName='this-job'", b);
r.j.assertLogContains("currentBuild.fullProjectName='this-folder/this-job'", b);
}
Expand Down