-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
[JENKINS-48923] Core should use UTF-8 by default #3231
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,6 +74,7 @@ | |
| import java.io.RandomAccessFile; | ||
| import java.io.Reader; | ||
| import java.nio.charset.Charset; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.text.DateFormat; | ||
| import java.text.SimpleDateFormat; | ||
| import java.util.ArrayList; | ||
|
|
@@ -267,6 +268,7 @@ private static enum State { | |
| * @since 1.257 | ||
| */ | ||
| protected String charset; | ||
| private transient Charset charsetInstance; | ||
|
|
||
| /** | ||
| * Keeps this log entries. | ||
|
|
@@ -551,8 +553,18 @@ public boolean isLogUpdated() { | |
| * @since 1.257 | ||
| */ | ||
| public final @Nonnull Charset getCharset() { | ||
| if(charset==null) return Charset.defaultCharset(); | ||
| return Charset.forName(charset); | ||
| if (charsetInstance==null) | ||
| return StandardCharsets.UTF_8; | ||
| return charsetInstance; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the charset in which the log file is written. | ||
| * @since TODO | ||
| */ | ||
| public final void setCharset(Charset charset) { | ||
| this.charsetInstance = charset; | ||
| this.charset = (charset==null ? null : charset.name()); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -1111,7 +1123,7 @@ public boolean getHasArtifacts() { | |
|
|
||
| private int addArtifacts(@Nonnull VirtualFile dir, | ||
| @Nonnull String path, @Nonnull String pathHref, | ||
| @Nonnull ArtifactList r, @Nonnull Artifact parent, int upTo) throws IOException { | ||
| @Nonnull ArtifactList r, Artifact parent, int upTo) throws IOException { | ||
| VirtualFile[] kids = dir.list(); | ||
| Arrays.sort(kids); | ||
|
|
||
|
|
@@ -1124,6 +1136,7 @@ private int addArtifacts(@Nonnull VirtualFile dir, | |
| boolean collapsed = (kids.length==1 && parent!=null); | ||
| Artifact a; | ||
| if (collapsed) { | ||
| assert parent!=null; | ||
| // Collapse single items into parent node where possible: | ||
| a = new Artifact(parent.getFileName() + '/' + child, childPath, | ||
| sub.isDirectory() ? null : childHref, length, | ||
|
|
@@ -1378,12 +1391,11 @@ public String toString() { | |
| } | ||
|
|
||
| String message = "No such file: " + logFile; | ||
| return new ByteArrayInputStream(charset != null ? message.getBytes(charset) : message.getBytes()); | ||
| return new ByteArrayInputStream(message.getBytes(getCharset())); | ||
| } | ||
|
|
||
| public @Nonnull Reader getLogReader() throws IOException { | ||
| if (charset==null) return new InputStreamReader(getLogInputStream()); | ||
| else return new InputStreamReader(getLogInputStream(),charset); | ||
| return new InputStreamReader(getLogInputStream(), getCharset()); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -1414,7 +1426,7 @@ public void writeLogTo(long offset, @Nonnull XMLOutput out) throws IOException { | |
| */ | ||
| public void writeWholeLogTo(@Nonnull OutputStream out) throws IOException, InterruptedException { | ||
| long pos = 0; | ||
| AnnotatedLargeText logText; | ||
| AnnotatedLargeText<Run<JobT, RunT>> logText; | ||
| logText = getLogText(); | ||
| pos = logText.writeLogTo(pos, out); | ||
|
|
||
|
|
@@ -1431,8 +1443,8 @@ public void writeWholeLogTo(@Nonnull OutputStream out) throws IOException, Inter | |
| * Used to URL-bind {@link AnnotatedLargeText}. | ||
| * @return A {@link Run} log with annotations | ||
| */ | ||
| public @Nonnull AnnotatedLargeText getLogText() { | ||
| return new AnnotatedLargeText(getLogFile(),getCharset(),!isLogUpdated(),this); | ||
| public @Nonnull AnnotatedLargeText<Run<JobT, RunT>> getLogText() { | ||
| return new AnnotatedLargeText<>(getLogFile(), getCharset(), !isLogUpdated(), this); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -1697,15 +1709,14 @@ protected final void execute(@Nonnull RunExecution job) { | |
| long start = System.currentTimeMillis(); | ||
|
|
||
| try { | ||
| Computer computer = Computer.currentComputer(); | ||
| if (computer != null) { | ||
| setCharset(computer.getDefaultCharset()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This part is what I think is really wrong. We should change the charset of all builds to be UTF-8, unconditionally, so that miscellaneous messages can always be safely written to the log file; but make sure that anything which copies external process output (namely, Cf. my corresponding changes for Pipeline in jenkinsci/workflow-job-plugin#27 and jenkinsci/durable-task-plugin#29, with integration test (
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am sure you are right. Ideally only UTF-8 would be used, though I was hesitant to go through and rip out anything relating to charsets. Right now I have changed the implementations of Computer.getDefaultCharset() to return |
||
| } | ||
| logger = createLogger(); | ||
| listener = createBuildListener(job, logger, getCharset()); | ||
|
|
||
| try { | ||
| Computer computer = Computer.currentComputer(); | ||
| Charset charset = null; | ||
| if (computer != null) { | ||
| charset = computer.getDefaultCharset(); | ||
| this.charset = charset.name(); | ||
| } | ||
| logger = createLogger(); | ||
| listener = createBuildListener(job, logger, charset); | ||
| listener.started(getCauses()); | ||
|
|
||
| Authentication auth = Jenkins.getAuthentication(); | ||
|
|
@@ -1869,7 +1880,7 @@ private void createSymlink(@Nonnull TaskListener listener, @Nonnull String name, | |
| /** | ||
| * Handles a fatal build problem (exception) that occurred during the build. | ||
| */ | ||
| private void handleFatalBuildProblem(@Nonnull BuildListener listener, @Nonnull Throwable e) { | ||
| private void handleFatalBuildProblem(BuildListener listener, @Nonnull Throwable e) { | ||
| if(listener!=null) { | ||
| LOGGER.log(FINE, getDisplayName()+" failed to build",e); | ||
|
|
||
|
|
@@ -2127,8 +2138,7 @@ public void doBuildNumber(StaplerResponse rsp) throws IOException { | |
| * Returns the build time stamp in the body. | ||
| */ | ||
| public void doBuildTimestamp( StaplerRequest req, StaplerResponse rsp, @QueryParameter String format) throws IOException { | ||
| rsp.setContentType("text/plain"); | ||
| rsp.setCharacterEncoding("US-ASCII"); | ||
| rsp.setContentType("text/plain;charset=UTF-8"); | ||
| rsp.setStatus(HttpServletResponse.SC_OK); | ||
| DateFormat df = format==null ? | ||
| DateFormat.getDateTimeInstance(DateFormat.SHORT,DateFormat.SHORT, Locale.ENGLISH) : | ||
|
|
@@ -2140,7 +2150,7 @@ public void doBuildTimestamp( StaplerRequest req, StaplerResponse rsp, @QueryPar | |
| * Sends out the raw console output. | ||
| */ | ||
| public void doConsoleText(StaplerRequest req, StaplerResponse rsp) throws IOException { | ||
| rsp.setContentType("text/plain;charset=UTF-8"); | ||
| rsp.setContentType("text/plain;charset=" + getCharset().name()); | ||
| try (InputStream input = getLogInputStream(); | ||
| OutputStream os = rsp.getCompressedOutputStream(req); | ||
| PlainTextConsoleOutputStream out = new PlainTextConsoleOutputStream(os)) { | ||
|
|
||
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.
Not enough.
onLoadmust also restorecharsetInstancebased on the persistedcharset, orgetCharsetwill wind up always returning UTF-8 after a restart.