-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Add JDK11 support and enable in CI #31644
Changes from 21 commits
d26bf2a
e132892
efe1844
3acd7b5
283b708
6091a3f
0060bb7
e433bce
be6fe54
dbe3a08
6e10477
aae1184
d7e299e
a7a49db
da094bc
06763cc
00d3b7e
1c19cf8
ef58e75
b744d1e
ba87083
fb05672
cfc5923
9961d8e
e6720b8
41d97c1
188460a
6d9fb3f
13f536b
062561c
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 |
---|---|---|
|
@@ -7,3 +7,4 @@ | |
|
||
ES_BUILD_JAVA: | ||
- java10 | ||
- java11 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,3 +8,4 @@ | |
ES_RUNTIME_JAVA: | ||
- java8 | ||
- java10 | ||
- java11 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,8 @@ import org.apache.tools.ant.taskdefs.condition.Os | |
import org.elasticsearch.gradle.LoggedExec | ||
import org.elasticsearch.gradle.Version | ||
|
||
import java.nio.charset.Charset | ||
|
||
import static org.elasticsearch.gradle.BuildPlugin.getJavaHome | ||
|
||
/** | ||
|
@@ -148,13 +150,20 @@ subprojects { | |
task buildBwcVersion(type: Exec) { | ||
dependsOn checkoutBwcBranch, writeBuildMetadata | ||
workingDir = checkoutDir | ||
// we are building branches that are officially built with JDK 8, push JAVA8_HOME to JAVA_HOME for these builds | ||
// also send RUNTIME_JAVA_HOME so the build doesn't fails on newer version the branch doesn't know about | ||
if (["5.6", "6.0", "6.1"].contains(bwcBranch)) { | ||
// we are building branches that are officially built with JDK 8, push JAVA8_HOME to JAVA_HOME for these builds | ||
environment('JAVA_HOME', getJavaHome(it, 8)) | ||
environment('RUNTIME_JAVA_HOME', getJavaHome(it, 8)) | ||
} else if ("6.2".equals(bwcBranch)) { | ||
environment('JAVA_HOME', getJavaHome(it, 9)) | ||
environment('RUNTIME_JAVA_HOME', getJavaHome(it, 9)) | ||
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. What is the reason for this change? I think it is because we can't run these builds when the run-time Java is JDK 11 and that is fine. If that's the case I think that we should lock these to the minimum supported JDK which is JDK 8. |
||
} else if (["6.3", "6.x"].contains(bwcBranch)) { | ||
environment('JAVA_HOME', getJavaHome(it, 10)) | ||
environment('RUNTIME_JAVA_HOME', getJavaHome(it, 10)) | ||
} else { | ||
environment('JAVA_HOME', project.compilerJavaHome) | ||
environment('RUNTIME_JAVA_HOME', project.compilerJavaHome) | ||
} | ||
if (Os.isFamily(Os.FAMILY_WINDOWS)) { | ||
executable 'cmd' | ||
|
@@ -177,6 +186,8 @@ subprojects { | |
} else if (showStacktraceName.equals("ALWAYS_FULL")) { | ||
args "--full-stacktrace" | ||
} | ||
standardOutput = new IdentingOutputStream(System.out) | ||
errorOutput = new IdentingOutputStream(System.err) | ||
doLast { | ||
List missing = artifactFiles.grep { file -> | ||
false == file.exists() | ||
|
@@ -196,3 +207,25 @@ subprojects { | |
} | ||
} | ||
} | ||
|
||
class IdentingOutputStream extends OutputStream { | ||
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. typo: Identing -> Indenting |
||
|
||
private final OutputStream delegate | ||
|
||
public IdentingOutputStream(OutputStream delegate) { | ||
this.delegate = delegate | ||
} | ||
|
||
@Override | ||
public void write(int b) { | ||
write([b] as int[], 0, 1) | ||
} | ||
|
||
public void write(int[] bytes, int offset, int length) { | ||
delegate.write( | ||
new String(bytes, offset, length) | ||
.replace("\n", "\n [bwc] ") | ||
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 think this is going to be pretty inefficient. Instead, you can loop over the bytes looking for newline. When you find a newline, emit the array up to that point. Then emit a fixed byte array you have with the newline and prefix. Then continue the loop. You won't need to create any temporary objects. |
||
.getBytes(Charset.forName("UTF-8")) | ||
) | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
db389ade95f48592908a84e7050a691c8834723c |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
9cef0aab8a4bb849a8476c058ce3ff302aba3fff |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
2507204241ab450456bdb8e8c0a8f986e418bd99 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -295,6 +295,7 @@ private Map<String, Object> parseDocument(String file, AttachmentProcessor proce | |
return attachmentData; | ||
} | ||
|
||
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/31305") | ||
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 think that we should use |
||
public void testIndexedChars() throws Exception { | ||
processor = new AttachmentProcessor(randomAlphaOfLength(10), "source_field", | ||
"target_field", EnumSet.allOf(AttachmentProcessor.Property.class), 19, false, null); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,7 @@ protected Collection<Class<? extends Plugin>> getPlugins() { | |
return pluginList(HdfsPlugin.class); | ||
} | ||
|
||
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/31498") | ||
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 think that we should use |
||
public void testSimpleWorkflow() { | ||
Client client = client(); | ||
|
||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
9cef0aab8a4bb849a8476c058ce3ff302aba3fff |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
2507204241ab450456bdb8e8c0a8f986e418bd99 |
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.
Why is this removed from forbidden apis? The forbidden signatures should against our minimum runtime, which is still java 8.
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.
The method was removed in jdk 11 and causing the test to fail.
Do we have per JDK version forbidden APIs ?
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.
We do not. I think this is related to running forbidden apis in a separate jvm. We should always be running it with the minimum supported version.
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.
Can you please comment this out and reference the issue to run forbidden apis in a forked jvm.
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.
Done, will remove comment once #31715 is fixed.
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.
If this issue was the only reason to no longer use the gradle plugin and fork instead, I can just say: The above forbidden-api signature can be removed anyways from elasticsearch (together with the previous one). Forbiddenapis has those signatures already in its "jdk-deprecated" ones, as it's a known problem - so there is no need to have it in Elasticsearch's custom signatures. "jdk-deprecated" automatically handles deprecations that were removed.
IMHO, #31715 was not a good idea, sorry!
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.
https://github.com/policeman-tools/forbidden-apis/blob/master/src/main/resources/de/thetaphi/forbiddenapis/signatures/jdk-deprecated-1.8.txt#L175-L180
and
https://github.com/policeman-tools/forbidden-apis/blob/master/src/main/resources/de/thetaphi/forbiddenapis/signatures/jdk-deprecated-1.8.txt#L7