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..7dcdf5f66d166 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 @@ -90,9 +90,6 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes assertUpperBound("9.2", "new_tv,8124000") } - /* - temporarily muted, see https://github.com/elastic/elasticsearch/pull/135226 - def "invalid changes to a upper bounds should be reverted"() { given: transportVersionUpperBound("9.2", "modification", "9000000") @@ -147,7 +144,7 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes assertReferableDefinitionDoesNotExist("test_tv") assertUpperBound("9.2", "existing_92,8123000") assertUpperBound("9.1", "existing_92,8012001") - }*/ + } def "a reference can be renamed"() { given: @@ -245,11 +242,8 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes def "unreferenced definitions are removed"() { given: referableTransportVersion("test_tv", "8124000,8012002") - /* - TODO: reset of upper bounds transportVersionUpperBound("9.2", "test_tv", "8124000") transportVersionUpperBound("9.1", "test_tv", "8012002") - */ when: def result = runGenerateAndValidateTask().build() @@ -412,8 +406,6 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes assertUpperBound("9.2", "new_tv,8124000") } - /* - TODO: reset of upper bounds def "deleted upper bounds files are restored"() { given: file("myserver/src/main/resources/transport/upper_bounds/9.2.csv").delete() @@ -424,7 +416,7 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes then: assertGenerateAndValidateSuccess(result) assertUpperBound("9.2", "existing_92,8123000") - }*/ + } def "upper bounds files must exist for backport branches"() { when: diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateTransportVersionDefinitionTask.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateTransportVersionDefinitionTask.java index d138ea717266e..4ba262ee16a10 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateTransportVersionDefinitionTask.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateTransportVersionDefinitionTask.java @@ -9,6 +9,7 @@ package org.elasticsearch.gradle.internal.transport; +import org.elasticsearch.gradle.internal.transport.TransportVersionResourcesService.IdAndDefinition; import org.gradle.api.DefaultTask; import org.gradle.api.file.ConfigurableFileCollection; import org.gradle.api.file.RegularFileProperty; @@ -31,6 +32,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; /** @@ -95,29 +97,39 @@ public abstract class GenerateTransportVersionDefinitionTask extends DefaultTask public void run() throws IOException { TransportVersionResourcesService resources = getResourceService().get(); Set referencedNames = TransportVersionReference.collectNames(getReferencesFiles()); - List changedDefinitionNames = resources.getChangedReferableDefinitionNames(); + Set changedDefinitionNames = resources.getChangedReferableDefinitionNames(); String targetDefinitionName = getTargetDefinitionName(resources, referencedNames, changedDefinitionNames); - getLogger().lifecycle("Generating transport version name: " + targetDefinitionName); + // First check for any unused definitions. This later generation to not get confused by a definition that can't be used. + removeUnusedNamedDefinitions(resources, referencedNames, changedDefinitionNames); + + Map> idsByBase = resources.getIdsByBase(); if (targetDefinitionName.isEmpty()) { - // TODO: resetting upper bounds needs to be done locally, otherwise it pulls in some (incomplete) changes from upstream main - // resetAllUpperBounds(resources); + getLogger().lifecycle("No transport version name detected, resetting upper bounds"); + resetAllUpperBounds(resources, idsByBase); } else { + getLogger().lifecycle("Generating transport version name: " + targetDefinitionName); List upstreamUpperBounds = resources.getUpperBoundsFromUpstream(); Set targetUpperBoundNames = getTargetUpperBoundNames(resources, upstreamUpperBounds, targetDefinitionName); - List ids = updateUpperBounds(resources, upstreamUpperBounds, targetUpperBoundNames, targetDefinitionName); + List ids = updateUpperBounds( + resources, + upstreamUpperBounds, + targetUpperBoundNames, + idsByBase, + targetDefinitionName + ); // (Re)write the definition file. resources.writeDefinition(new TransportVersionDefinition(targetDefinitionName, ids, true)); } - removeUnusedNamedDefinitions(resources, referencedNames, changedDefinitionNames); } private List updateUpperBounds( TransportVersionResourcesService resources, List existingUpperBounds, Set targetUpperBoundNames, + Map> idsByBase, String definitionName ) throws IOException { String currentUpperBoundName = getCurrentUpperBoundName().get(); @@ -146,9 +158,9 @@ private List updateUpperBounds( resources.writeUpperBound(newUpperBound, stageInGit); } ids.add(targetId); - } else { + } else if (resources.getChangedUpperBoundNames().contains(upperBoundName)) { // Default case: we're not targeting this branch so reset it - resources.writeUpperBound(existingUpperBound, false); + resetUpperBound(resources, existingUpperBound, idsByBase, definitionName); } } @@ -159,7 +171,7 @@ private List updateUpperBounds( private String getTargetDefinitionName( TransportVersionResourcesService resources, Set referencedNames, - List changedDefinitions + Set changedDefinitions ) { if (getDefinitionName().isPresent()) { // an explicit name was passed in, so use it @@ -180,7 +192,7 @@ private String getTargetDefinitionName( if (changedDefinitions.isEmpty()) { return ""; } else { - String changedDefinitionName = changedDefinitions.getFirst(); + String changedDefinitionName = changedDefinitions.iterator().next(); if (referencedNames.contains(changedDefinitionName)) { return changedDefinitionName; } else { @@ -248,16 +260,38 @@ private Set getUpperBoundNamesFromDefinition( return upperBoundNames; } - private void resetAllUpperBounds(TransportVersionResourcesService resources) throws IOException { - for (TransportVersionUpperBound upperBound : resources.getUpperBoundsFromUpstream()) { - resources.writeUpperBound(upperBound, false); + private void resetAllUpperBounds(TransportVersionResourcesService resources, Map> idsByBase) + throws IOException { + for (String upperBoundName : resources.getChangedUpperBoundNames()) { + TransportVersionUpperBound upstreamUpperBound = resources.getUpperBoundFromUpstream(upperBoundName); + resetUpperBound(resources, upstreamUpperBound, idsByBase, null); + } + } + + private void resetUpperBound( + TransportVersionResourcesService resources, + TransportVersionUpperBound upperBound, + Map> idsByBase, + String ignoreDefinitionName + ) throws IOException { + List idsForUpperBound = idsByBase.get(upperBound.definitionId().base()); + if (idsForUpperBound == null) { + throw new RuntimeException("Could not find base id: " + upperBound.definitionId().base()); + } + IdAndDefinition resetValue = idsForUpperBound.getLast(); + if (resetValue.definition().name().equals(ignoreDefinitionName)) { + // there must be another definition in this base since the ignored definition is new + assert idsForUpperBound.size() >= 2; + resetValue = idsForUpperBound.get(idsForUpperBound.size() - 2); } + var resetUpperBound = new TransportVersionUpperBound(upperBound.name(), resetValue.definition().name(), resetValue.id()); + resources.writeUpperBound(resetUpperBound, false); } private void removeUnusedNamedDefinitions( TransportVersionResourcesService resources, Set referencedNames, - List changedDefinitions + Set changedDefinitions ) throws IOException { for (String definitionName : changedDefinitions) { if (referencedNames.contains(definitionName) == false) { 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..5be53829d06f8 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 @@ -26,6 +26,7 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.Collections; +import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -33,6 +34,7 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiFunction; +import java.util.function.Consumer; import java.util.stream.Collectors; import javax.inject.Inject; @@ -66,6 +68,8 @@ public interface Parameters extends BuildServiceParameters { Property getUpstreamRefOverride(); } + record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definition) {} + @Inject public abstract ExecOperations getExecOperations(); @@ -129,19 +133,8 @@ TransportVersionDefinition getReferableDefinitionFromUpstream(String name) { } /** Get the definition names which have local changes relative to upstream */ - List getChangedReferableDefinitionNames() { - List changedDefinitions = new ArrayList<>(); - // make sure the prefix is git style paths, always forward slashes - String referablePrefix = REFERABLE_DIR.toString().replace('\\', '/'); - for (String changedPath : getChangedResources()) { - if (changedPath.contains(referablePrefix) == false) { - continue; - } - int lastSlashNdx = changedPath.lastIndexOf('/'); - String name = changedPath.substring(lastSlashNdx + 1, changedPath.length() - 4 /* .csv */); - changedDefinitions.add(name); - } - return changedDefinitions; + Set getChangedReferableDefinitionNames() { + return getChangedNames(REFERABLE_DIR); } /** Test whether the given referable definition exists */ @@ -187,6 +180,27 @@ TransportVersionDefinition getUnreferableDefinitionFromUpstream(String name) { return getUpstreamFile(resourcePath, (path, contents) -> TransportVersionDefinition.fromString(path, contents, false)); } + /** Get all the ids and definitions for a given base id, sorted by the complete id */ + Map> getIdsByBase() throws IOException { + Map> idsByBase = new HashMap<>(); + + // first collect all ids, organized by base + Consumer addToBase = definition -> { + for (TransportVersionId id : definition.ids()) { + idsByBase.computeIfAbsent(id.base(), k -> new ArrayList<>()).add(new IdAndDefinition(id, definition)); + } + }; + getReferableDefinitions().values().forEach(addToBase); + getUnreferableDefinitions().values().forEach(addToBase); + + // now sort the ids within each base so we can check density later + for (var ids : idsByBase.values()) { + // first sort the ids list so we can check compactness and quickly lookup the highest id later + ids.sort(Comparator.comparingInt(a -> a.id().complete())); + } + return idsByBase; + } + /** Read all upper bound files and return them mapped by their release name */ Map getUpperBounds() throws IOException { Map upperBounds = new HashMap<>(); @@ -220,6 +234,10 @@ List getUpperBoundsFromUpstream() throws IOException return upperBounds; } + Set getChangedUpperBoundNames() { + return getChangedNames(UPPER_BOUNDS_DIR); + } + /** Write the given upper bound to a file in the transport resources */ void writeUpperBound(TransportVersionUpperBound upperBound, boolean stageInGit) throws IOException { Path path = transportResourcesDir.resolve(getUpperBoundRelativePath(upperBound.name())); @@ -317,6 +335,21 @@ private Set getChangedResources() { return changedResources.get(); } + private Set getChangedNames(Path resourcesDir) { + Set changedNames = new HashSet<>(); + // make sure the prefix is git style paths, always forward slashes + String resourcesPrefix = resourcesDir.toString().replace('\\', '/'); + for (String changedPath : getChangedResources()) { + if (changedPath.contains(resourcesPrefix) == false) { + continue; + } + int lastSlashNdx = changedPath.lastIndexOf('/'); + String name = changedPath.substring(lastSlashNdx + 1, changedPath.length() - 4 /* .csv */); + changedNames.add(name); + } + return changedNames; + } + // Read a transport version resource from the upstream, or return null if it doesn't exist there private T getUpstreamFile(Path resourcePath, BiFunction parser) { String pathString = resourcePath.toString().replace('\\', '/'); // normalize to forward slash that git uses diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java index 8a69158ff4e07..02338d7f677f1 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java @@ -11,6 +11,7 @@ import com.google.common.collect.Comparators; +import org.elasticsearch.gradle.internal.transport.TransportVersionResourcesService.IdAndDefinition; import org.gradle.api.DefaultTask; import org.gradle.api.file.ConfigurableFileCollection; import org.gradle.api.provider.Property; @@ -28,8 +29,6 @@ import java.io.IOException; import java.nio.file.Path; -import java.util.ArrayList; -import java.util.Collection; import java.util.Comparator; import java.util.HashMap; import java.util.List; @@ -67,8 +66,6 @@ public Path getResourcesDir() { @Input public abstract Property getCurrentUpperBoundName(); - private record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definition) {} - private static final Pattern NAME_FORMAT = Pattern.compile("[a-z0-9_]+"); @ServiceReference("transportVersionResources") @@ -81,7 +78,7 @@ public void validateTransportVersions() throws IOException { Map referableDefinitions = resources.getReferableDefinitions(); Map unreferableDefinitions = resources.getUnreferableDefinitions(); Map allDefinitions = collectAllDefinitions(referableDefinitions, unreferableDefinitions); - Map> idsByBase = collectIdsByBase(allDefinitions.values()); + Map> idsByBase = resources.getIdsByBase(); Map upperBounds = resources.getUpperBounds(); TransportVersionUpperBound currentUpperBound = upperBounds.get(getCurrentUpperBoundName().get()); boolean onReleaseBranch = checkIfDefinitelyOnReleaseBranch(upperBounds); @@ -129,25 +126,6 @@ private Map collectAllDefinitions( return allDefinitions; } - private Map> collectIdsByBase(Collection definitions) { - Map> idsByBase = new HashMap<>(); - - // first collect all ids, organized by base - for (TransportVersionDefinition definition : definitions) { - for (TransportVersionId id : definition.ids()) { - idsByBase.computeIfAbsent(id.base(), k -> new ArrayList<>()).add(new IdAndDefinition(id, definition)); - } - } - - // now sort the ids within each base so we can check density later - for (var ids : idsByBase.values()) { - // first sort the ids list so we can check compactness and quickly lookup the highest id later - ids.sort(Comparator.comparingInt(a -> a.id().complete())); - } - - return idsByBase; - } - private void validateNamedDefinition(TransportVersionDefinition definition, Set referencedNames) { if (referencedNames.contains(definition.name()) == false) { throwDefinitionFailure(definition, "is not referenced"); @@ -280,10 +258,10 @@ private void validateBase(int base, List ids) { IdAndDefinition current = ids.get(ndx); if (previous.id().equals(current.id())) { - Path existingDefinitionPath = getResources().get().getDefinitionPath(previous.definition); + Path existingDefinitionPath = getResources().get().getDefinitionPath(previous.definition()); throwDefinitionFailure( current.definition(), - "contains id " + current.id + " already defined in [" + existingDefinitionPath + "]" + "contains id " + current.id() + " already defined in [" + existingDefinitionPath + "]" ); }