From e1640607f42c990756a1b7f6e10c5c57fa07f5b2 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 26 Sep 2025 20:46:01 -0700 Subject: [PATCH 1/3] Use merge base to find original version of transport resources Transport version generation relies on finding the original contents of transport resources before they may have been modified by the current branch. Currently it looks directly at the upstream main branch. Yet that means newly generated ids could leave holes if the branch is out of date with upstream main. Instead, this commit now uses the merge base of the branch. The merge base is calculcated against upstream main. This allows generation to be locally consistent with the branch. The generated id may already be used on main, but that will be detected by a merge conflict in the upper bound before merging the PR. Additionally if in the middle of a merge, like with resolveTransportVersionConflict, the MERGE_HEAD is used as the base so that the regenerated ids will be based on the state at the end of the merge. --- .../AbstractTransportVersionFuncTest.groovy | 2 +- ...lveTransportVersionConflictFuncTest.groovy | 31 +++++++ .../TransportVersionGenerationFuncTest.groovy | 17 ++++ .../TransportVersionResourcesService.java | 93 +++++++++++-------- .../fixtures/AbstractGradleFuncTest.groovy | 4 +- 5 files changed, 104 insertions(+), 43 deletions(-) diff --git a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy index d97c3747ce343..3d1f397406f3e 100644 --- a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy +++ b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy @@ -116,7 +116,7 @@ class AbstractTransportVersionFuncTest extends AbstractGradleFuncTest { include ':myplugin' """ propertiesFile << """ - org.elasticsearch.transports.upstreamRef=main + org.elasticsearch.transport.upstreamRef=main """ versionPropertiesFile.text = versionPropertiesFile.text.replace("9.1.0", "9.2.0") diff --git a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/ResolveTransportVersionConflictFuncTest.groovy b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/ResolveTransportVersionConflictFuncTest.groovy index 974637020ea37..2f009ad5d9e2a 100644 --- a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/ResolveTransportVersionConflictFuncTest.groovy +++ b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/ResolveTransportVersionConflictFuncTest.groovy @@ -96,4 +96,35 @@ class ResolveTransportVersionConflictFuncTest extends AbstractTransportVersionFu assertResolveAndValidateSuccess(result) assertUpperBound("9.2", "existing_92,8123000") } + + def "upstream changes don't affect merge"() { + given: + // setup main with 2 commits, but we will only merge in the first one + execute("git checkout main") + referableAndReferencedTransportVersion("upstream_new_tv1", "8124000") + transportVersionUpperBound("9.2", "upstream_new_tv1", "8124000") + execute("git add .") + execute("git commit -m update1") + String toMerge = execute("git rev-parse HEAD") + referableAndReferencedTransportVersion("upstream_new_tv2", "8125000") + transportVersionUpperBound("9.2", "upstream_new_tv2", "8125000") + execute("git add .") + execute("git commit -m update2") + execute("git checkout test") + // now commit a conflict on the test branch, a new TV + referableAndReferencedTransportVersion("branch_new_tv", "8124000") + transportVersionUpperBound("9.2", "branch_new_tv", "8124000") + execute("git add .") + execute("git commit -m branch") + // and finally initiate the merge + System.out.println("Merging commit " + toMerge); + execute("git merge " + toMerge, testProjectDir.root, true); + + when: + def result = runResolveAndValidateTask().build() + + then: + assertResolveAndValidateSuccess(result) + assertUpperBound("9.2", "branch_new_tv,8125000") + } } diff --git a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy index a5d7abed3c0ac..163397485313c 100644 --- a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy +++ b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy @@ -504,4 +504,21 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes assertUpperBound("9.2", "new_tv,8124000") assertReferableDefinition("new_tv", "8124000") } + + def "generation is idempotent on upstream changes"() { + given: + execute("git checkout main") + referableAndReferencedTransportVersion("new_tv", "8124000") + transportVersionUpperBound("9.2", "new_tv", "8124000") + execute("git add .") + execute("git commit -m update") + execute("git checkout test") + + when: + def result = runGenerateAndValidateTask().build() + + then: + assertGenerateAndValidateSuccess(result) + assertUpperBound("9.2", "existing_92,8123000") + } } diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java index 251f514002da6..74fd17d21887b 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java @@ -76,7 +76,8 @@ public interface Parameters extends BuildServiceParameters { private final Path transportResourcesDir; private final Path rootDir; - private final AtomicReference upstreamRefName = new AtomicReference<>(); + private final String upstreamRefOverride; + private final AtomicReference baseRefName = new AtomicReference<>(); private final AtomicReference> upstreamResources = new AtomicReference<>(null); private final AtomicReference> changedResources = new AtomicReference<>(null); @@ -84,9 +85,7 @@ public interface Parameters extends BuildServiceParameters { public TransportVersionResourcesService(Parameters params) { this.transportResourcesDir = params.getTransportResourcesDirectory().get().getAsFile().toPath(); this.rootDir = params.getRootDirectory().get().getAsFile().toPath(); - if (params.getUpstreamRefOverride().isPresent()) { - upstreamRefName.set(params.getUpstreamRefOverride().get()); - } + upstreamRefOverride = params.getUpstreamRefOverride().getOrNull(); } /** @@ -240,52 +239,66 @@ private Path getUpperBoundRelativePath(String name) { return UPPER_BOUNDS_DIR.resolve(name + ".csv"); } - private String getUpstreamRefName() { - if (upstreamRefName.get() == null) { - synchronized (upstreamRefName) { - String remotesOutput = gitCommand("remote").strip(); - + private String getBaseRefName() { + if (baseRefName.get() == null) { + synchronized (baseRefName) { String refName; - if (remotesOutput.isEmpty()) { - refName = "main"; // fallback to local main if no remotes, this happens in tests + // the existence of the MERGE_HEAD ref means we are in the middle of a merge, and should use that as our base + String gitDir = gitCommand("rev-parse", "--git-dir").strip(); + if (Files.exists(Path.of(gitDir).resolve("MERGE_HEAD"))) { + refName = gitCommand("rev-parse", "--verify", "MERGE_HEAD").strip(); } else { - List remoteNames = List.of(remotesOutput.split("\n")); - String transportVersionRemoteName = "transport-version-resources-upstream"; - if (remoteNames.contains(transportVersionRemoteName) == false) { - // our special remote doesn't exist yet, so create it - String upstreamUrl = null; - for (String remoteName : remoteNames) { - String getUrlOutput = gitCommand("remote", "get-url", remoteName).strip(); - if (getUrlOutput.startsWith("git@github.com:elastic/") - || getUrlOutput.startsWith("https://github.com/elastic/")) { - upstreamUrl = getUrlOutput; - } - } - - if (upstreamUrl != null) { - gitCommand("remote", "add", transportVersionRemoteName, upstreamUrl); - } else { - throw new RuntimeException("No elastic github remotes found to copy"); - } - } - - // make sure the remote main ref is up to date - gitCommand("fetch", transportVersionRemoteName, "main"); - - refName = transportVersionRemoteName + "/main"; + String upstreamRef = findUpstreamRef(); + refName = gitCommand("merge-base", upstreamRef, "HEAD").strip(); + } + + baseRefName.set(refName); + } + } + return baseRefName.get(); + } + + private String findUpstreamRef() { + if (upstreamRefOverride != null) { + return upstreamRefOverride; + } + + String remotesOutput = gitCommand("remote").strip(); + if (remotesOutput.isEmpty()) { + throw new RuntimeException("No remotes found. If this is a test set gradle property " + + "org.elasticsearch.transport.upstreamRef"); + } + List remoteNames = List.of(remotesOutput.split("\n")); + String transportVersionRemoteName = "transport-version-resources-upstream"; + if (remoteNames.contains(transportVersionRemoteName) == false) { + // our special remote doesn't exist yet, so create it + String upstreamUrl = null; + for (String remoteName : remoteNames) { + String getUrlOutput = gitCommand("remote", "get-url", remoteName).strip(); + if (getUrlOutput.startsWith("git@github.com:elastic/") + || getUrlOutput.startsWith("https://github.com/elastic/")) { + upstreamUrl = getUrlOutput; } - upstreamRefName.set(refName); + } + if (upstreamUrl != null) { + gitCommand("remote", "add", transportVersionRemoteName, upstreamUrl); + } else { + throw new RuntimeException("No elastic github remotes found to copy"); } } - return upstreamRefName.get(); + + // make sure the remote main ref is up to date + gitCommand("fetch", transportVersionRemoteName, "main"); + + return transportVersionRemoteName + "/main"; } // Return the transport version resources paths that exist in upstream private Set getUpstreamResources() { if (upstreamResources.get() == null) { synchronized (upstreamResources) { - String output = gitCommand("ls-tree", "--name-only", "-r", getUpstreamRefName(), "."); + String output = gitCommand("ls-tree", "--name-only", "-r", getBaseRefName(), "."); HashSet resources = new HashSet<>(); Collections.addAll(resources, output.split("\n")); // git always outputs LF @@ -301,7 +314,7 @@ private Set getChangedResources() { synchronized (changedResources) { HashSet resources = new HashSet<>(); - String diffOutput = gitCommand("diff", "--name-only", "--relative", getUpstreamRefName(), "."); + String diffOutput = gitCommand("diff", "--name-only", "--relative", getBaseRefName(), "."); if (diffOutput.strip().isEmpty() == false) { Collections.addAll(resources, diffOutput.split("\n")); // git always outputs LF } @@ -324,7 +337,7 @@ private T getUpstreamFile(Path resourcePath, BiFunction par return null; } - String content = gitCommand("show", getUpstreamRefName() + ":./" + pathString).strip(); + String content = gitCommand("show", getBaseRefName() + ":./" + pathString).strip(); return parser.apply(resourcePath, content); } diff --git a/build-tools/src/testFixtures/groovy/org/elasticsearch/gradle/fixtures/AbstractGradleFuncTest.groovy b/build-tools/src/testFixtures/groovy/org/elasticsearch/gradle/fixtures/AbstractGradleFuncTest.groovy index 553d5ea7d8d40..6109979df0331 100644 --- a/build-tools/src/testFixtures/groovy/org/elasticsearch/gradle/fixtures/AbstractGradleFuncTest.groovy +++ b/build-tools/src/testFixtures/groovy/org/elasticsearch/gradle/fixtures/AbstractGradleFuncTest.groovy @@ -211,10 +211,10 @@ abstract class AbstractGradleFuncTest extends Specification { """ } - String execute(String command, File workingDir = testProjectDir.root) { + String execute(String command, File workingDir = testProjectDir.root, boolean ignoreFailure = false) { def proc = command.execute(Collections.emptyList(), workingDir) proc.waitFor() - if (proc.exitValue()) { + if (proc.exitValue() && ignoreFailure == false) { String msg = """Error running command ${command}: Sysout: ${proc.inputStream.text} Syserr: ${proc.errorStream.text} From b8e0bb83ac6696b5da1854d5de3bbd619ff5a604 Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Sat, 27 Sep 2025 04:00:48 +0000 Subject: [PATCH 2/3] [CI] Auto commit changes from spotless --- .../transport/TransportVersionResourcesService.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java index 74fd17d21887b..71ff8e87b2e41 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java @@ -265,8 +265,9 @@ private String findUpstreamRef() { String remotesOutput = gitCommand("remote").strip(); if (remotesOutput.isEmpty()) { - throw new RuntimeException("No remotes found. If this is a test set gradle property " + - "org.elasticsearch.transport.upstreamRef"); + throw new RuntimeException( + "No remotes found. If this is a test set gradle property " + "org.elasticsearch.transport.upstreamRef" + ); } List remoteNames = List.of(remotesOutput.split("\n")); String transportVersionRemoteName = "transport-version-resources-upstream"; @@ -275,8 +276,7 @@ private String findUpstreamRef() { String upstreamUrl = null; for (String remoteName : remoteNames) { String getUrlOutput = gitCommand("remote", "get-url", remoteName).strip(); - if (getUrlOutput.startsWith("git@github.com:elastic/") - || getUrlOutput.startsWith("https://github.com/elastic/")) { + if (getUrlOutput.startsWith("git@github.com:elastic/") || getUrlOutput.startsWith("https://github.com/elastic/")) { upstreamUrl = getUrlOutput; } } From 8783b2c2de376a3b06ebfe1d792be04dc7cb1c4d Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Mon, 29 Sep 2025 15:00:50 -0700 Subject: [PATCH 3/3] constant --- .../transport/TransportVersionResourcesService.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java index 71ff8e87b2e41..e1801022fa73f 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java @@ -56,6 +56,7 @@ public abstract class TransportVersionResourcesService implements BuildService { private static final Logger logger = Logging.getLogger(TransportVersionResourcesService.class); + private static final String UPSTREAM_REMOTE_NAME = "transport-version-resources-upstream"; public interface Parameters extends BuildServiceParameters { DirectoryProperty getTransportResourcesDirectory(); @@ -270,8 +271,7 @@ private String findUpstreamRef() { ); } List remoteNames = List.of(remotesOutput.split("\n")); - String transportVersionRemoteName = "transport-version-resources-upstream"; - if (remoteNames.contains(transportVersionRemoteName) == false) { + if (remoteNames.contains(UPSTREAM_REMOTE_NAME) == false) { // our special remote doesn't exist yet, so create it String upstreamUrl = null; for (String remoteName : remoteNames) { @@ -282,16 +282,16 @@ private String findUpstreamRef() { } if (upstreamUrl != null) { - gitCommand("remote", "add", transportVersionRemoteName, upstreamUrl); + gitCommand("remote", "add", UPSTREAM_REMOTE_NAME, upstreamUrl); } else { throw new RuntimeException("No elastic github remotes found to copy"); } } // make sure the remote main ref is up to date - gitCommand("fetch", transportVersionRemoteName, "main"); + gitCommand("fetch", UPSTREAM_REMOTE_NAME, "main"); - return transportVersionRemoteName + "/main"; + return UPSTREAM_REMOTE_NAME + "/main"; } // Return the transport version resources paths that exist in upstream