Skip to content

Fix backwards compatibility with pre-Java11 build-tools users #45633

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
merged 1 commit into from
Aug 16, 2019
Merged

Fix backwards compatibility with pre-Java11 build-tools users #45633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 2 additions & 8 deletions buildSrc/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,6 @@ sourceSets {
minimumRuntime { }
}

allprojects {
apply plugin: 'java'
targetCompatibility = 11
sourceCompatibility = 11
}

configurations {
reaper
}
Expand Down Expand Up @@ -160,8 +154,8 @@ if (project != rootProject) {
apply plugin: 'nebula.maven-scm'

allprojects {
targetCompatibility = 11
sourceCompatibility = 11
targetCompatibility = 10
sourceCompatibility = 10
Copy link
Member

@jasontedor jasontedor Aug 15, 2019

Choose a reason for hiding this comment

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

Doesn’t the source/target compatibility need to be 8 not 10, otherwise we can still get into a trouble if an API from JDK 9 or 10 is used?

Copy link
Contributor Author

@mark-vieira mark-vieira Aug 15, 2019

Choose a reason for hiding this comment

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

To be quite honest, It's not clear to me why this is 10 or if it needs to be. There are funny interactions here between this and the Groovy plugin as well. This is simply a revert back to what this was before the problematic commit that caused the hadoop builds to begin exploding. I think it's likely we use Java9+ APIs in .java code but that isn't used by elasticsearch-hadoop at runtime so all is well.

This is admittedly very awkward and the true solution is to fix the hadoop build to bring it up to Gradle 5.x but we've been kicking that can down the road. This PR is just to unblock the snapshot release builds for the time being. Essentially, this is just the minimal change to bring us back to a known working state.

Copy link
Member

@jasontedor jasontedor Aug 15, 2019

Choose a reason for hiding this comment

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

Well, in that case, I think we need to stop kicking the can down the road. I don't like that we are held back from using modern features because of this. I also don't like that we're making a change it seems we don't fully understand (myself included)? I don't think this change is going to prevent us from making this mistake yet again, breaking important downstream builds for days.

Copy link
Contributor Author

@mark-vieira mark-vieira Aug 15, 2019

Choose a reason for hiding this comment

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

💯 We had a chat about sorting out the hadoop stuff while back, we just need to prioritize that. I'll put this on the top of my list for when I return from PTO.

Copy link
Member

@jasontedor jasontedor Aug 15, 2019

Choose a reason for hiding this comment

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

Thanks @mark-vieira. ❤️

}

// groovydoc succeeds, but has some weird internal exception...
Expand Down
2 changes: 2 additions & 0 deletions buildSrc/reaper/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
apply plugin: 'java'

jar {
archiveName = "${project.name}.jar"
manifest {
Expand Down
8 changes: 5 additions & 3 deletions buildSrc/src/main/java/org/elasticsearch/gradle/ReaperService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,15 @@
import org.gradle.api.logging.Logger;
import org.gradle.internal.jvm.Jvm;

import java.io.FileWriter;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.UncheckedIOException;
import java.net.URL;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

Expand Down Expand Up @@ -68,8 +70,8 @@ public void registerPid(String serviceId, long pid) {
public void registerCommand(String serviceId, String... command) {
ensureReaperStarted();

try {
Files.writeString(getCmdFile(serviceId), String.join(" ", command));
try (FileWriter writer = new FileWriter(getCmdFile(serviceId).toFile())) {
writer.write(String.join(" ", command));
} catch (IOException e) {
throw new UncheckedIOException(e);
}
Expand Down Expand Up @@ -143,7 +145,7 @@ private Path locateReaperJar() {

if (matcher.matches()) {
String path = matcher.group(1);
return Path.of(
return Paths.get(
OS.<String>conditional()
.onWindows(() -> path.substring(1))
.onUnix(() -> path)
Expand Down