From 2948eddd50b753ab506cc3e023205e530f46717b Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 30 Sep 2025 11:39:16 -0700 Subject: [PATCH] Use merge base to find original version of transport resources (#135564) 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 7dcdf5f66d166..0f2571d7ede9d 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 @@ -496,4 +496,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 5be53829d06f8..89b4cf67ce6e5 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 @@ -58,6 +58,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(); @@ -80,7 +81,8 @@ record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definit 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); @@ -88,9 +90,7 @@ record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definit 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(); } /** @@ -258,52 +258,65 @@ 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")); + if (remoteNames.contains(UPSTREAM_REMOTE_NAME) == 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", UPSTREAM_REMOTE_NAME, 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", UPSTREAM_REMOTE_NAME, "main"); + + return UPSTREAM_REMOTE_NAME + "/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 @@ -319,7 +332,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 } @@ -357,7 +370,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}