Bwc changes #28505

hub-cap · 2018-02-02T22:13:40Z

Generalizing BWC building so that there is less code to modify for a release. This ensures we do not
need to think about what major or minor version is in the gradle code. It follows the general rules of the
elastic release structure. For more information on the rules, see the VersionCollection's javadoc.

This also removes the additional bwc snapshots that will never be released, such as 6.0.2, which were
being built and tested against every time we ran bwc tests.

Additionally, it creates 4 new projects that correspond to the different types of snapshots that may exist
for a given version. Its possible to now run those individual tasks to work out bwc logic whereas
previously it was impossible and the entire suite of bwc tests had to be run to work out any logic changes
in the build tools' bwc project. Please note that if the project does not make sense for the version that is
current, that an error will be thrown from that individual project if an attempt is made to run it.

This should allow for automating the version bumps as well, since it removes all the hardcoded version
logic from the configs.

…i made

DaveCTurner

Thanks @hub-cap so much for picking this up. I'm on my way to the airport, so have only had time for a quick look.

I left a handful of comments on things that jumped out at me, but haven't had time to think through the changes to the logic and may not do so for another week. The fact that there are now tests around VersionCollection is great, and makes this whole area less worrying.

I felt it worth noting that ca70ca6 was a PITA - it addressed failures that only occurred during release builds which are waaay down the build pipeline. I think this preserves that fix but it's one that'll need keeping an eye on.

DaveCTurner · 2018-02-03T08:57:09Z

build.gradle

+  addBWCSnapshotSubstitutions(':distribution:bwc:staged-minor-snapshot', versionCollection.stagedMinorSnapshot)
+  addBWCSnapshotSubstitutions(':distribution:bwc:next-bugfix-snapshot', versionCollection.nextBugfixSnapshot)
+  addBWCSnapshotSubstitutions(':distribution:bwc:maintenance-bugfix-snapshot', versionCollection.maintenanceBugfixSnapshot)
+


I like this, but now I see it I wonder about the duplication between the Gradle project name and the property on VersionCollection. Can that be moved entirely into VersionCollection?

Ill see what I can do. Im not sure I like passing in the project ext to the VC project, which is used in the addBWCSnapshotSubstitutions method.. but it would be nice to be able to (like the bottom comment about the ifs in the bwc/build.gradle) consolidate the mapping between those in some single place. ill eyeball it, I think it can be better.

DaveCTurner · 2018-02-03T08:57:58Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/Version.groovy

@@ -34,7 +34,6 @@ public class Version {
    final int revision
    final int id
    final boolean snapshot
-    final String branch
    /**
     * Suffix on the version name. Unlike Version.java the build does not
     * consider alphas and betas different versions, it just preserves the


It looks like we now do consider alphas and betas so this comment is out-of-date.

DaveCTurner · 2018-02-03T08:59:11Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/VersionCollection.groovy

@@ -25,159 +25,293 @@ import java.util.regex.Matcher

 /**
 * The collection of version constants declared in Version.java, for use in BWC testing.
+ *
+ *  if major+1 released: released artifacts from $version down to major-1.highestMinor.highestPatch, none of these should be snapshots, period.


This seems to relate major+1 to major-1, which are two major versions apart. I think one of these should just be major.

Ahh yea i can fix this up. Its because major is current version major, but it does not really say that well so its confusing ++

ahh, ok so this should be as is, since its saying if your next major version is released (IE, im on 5.x currently), then go from $version (some 5.x) down to 2.whatever_the_highest_minor_is, which is the wire compat guarantee. It is slightly confusing tho, and i welcome a rewording help, i just cant think of a better way currently.

DaveCTurner · 2018-02-03T09:00:49Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/VersionCollection.groovy

-                        Integer.parseInt(match.group(3)), (match.group(4) ?: '').replace('_', '-'), false, null)
-
-                if (versions.size() > 0 && foundVersion.onOrBeforeIncludingSuffix(versions[-1])) {
-                    throw new GradleException("Versions.java contains out of order version constants:" +


I can't see this check any more, but ISTR it's important. Maybe it's elsewhere, I can't remember right now.

Its a treemap now, so it sorts it. there is not really a way i can check. i think it was important so we didnt have to sort it in the code below where that original check was.

DaveCTurner · 2018-02-03T09:02:19Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/VersionCollection.groovy


    /**
-     * Construct a VersionCollection from the lines of the Version.java file.
+     * Construct a VersionCollection from the lines of the Version.java file. The basic logic for the following is pretty straight forward.


A bold claim :)

HAHAHAHA ++

DaveCTurner · 2018-02-03T09:03:11Z

buildSrc/src/test/groovy/org/elasticsearch/gradle/VersionCollectionTest.groovy

@@ -0,0 +1,226 @@
+package org.elasticsearch.gradle
+
+class VersionCollectionTest extends GroovyTestCase {


Hooray! 🎉

DaveCTurner · 2018-02-03T09:04:36Z

distribution/bwc/build.gradle

+  } else if (project.name == 'maintenance-bugfix-snapshot') {
+    bwcVersion = versionCollection.maintenanceBugfixSnapshot
+  } else if (project.name == 'next-bugfix-snapshot') {
+    bwcVersion = versionCollection.nextBugfixSnapshot


As above, I think it'd be nice to move the logic that relates Gradle project names and the versions all the way into VersionCollection.

yea i think its a good idea to leave it as a helper method in the VC code

DaveCTurner · 2018-02-03T09:07:18Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/VersionCollection.groovy

+    /**
+     * Helper function for turning a version into a snapshot version, removing and readding it to the tree
+     */
+    private Version removeAndReaddAsSnapshot(Version version) {


Typo: too many ds.

thats actually Re-Add, which after re-reading, is a terrible name.

Why not just replaceAsSnapshot?

hub-cap · 2018-02-03T13:55:45Z

I felt it worth noting that ca70ca6 was a PITA - it addressed failures that only occurred during release builds which are waaay down the build pipeline. I think this preserves that fix but it's one that'll need keeping an eye on.

I have a feeling Ill be eyeballing the next few of these releases :) And bonus, now you get out of doing it @DaveCTurner !!

hub-cap · 2018-02-03T13:58:42Z

Also, @DaveCTurner when you do review, pls note that the goal of this was to try to keep it consolidated, but more legible than the original way it was done before you had to slog through it to understand it. The projects backing git branches will never change anymore like they used to, so its easier to reason about which of the snapshots is active for a given current branch.

EX 6.1 does not have a nextMinor (only currentversion == unreleased major do), nor a staged (cuz its been released), nor a nextBugfix (released), but does have a maint (5.6 snap).

EX 6.x could have a staged (if 6.2 is unreleased), def has a nextBugfix (6.1 snap) and maint (5.6 snap)

hub-cap · 2018-02-05T14:42:47Z

@DaveCTurner ive addressed your comments, go-head have a look see while I figure out whats up w/ the CI tests failing :)

rjernst

This looks good in general. I left some minor suggestions and thoughts. For the descriptions of the different version terminology, I would have someone look at them who knows nothing about this code.

rjernst · 2018-02-08T16:19:08Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/VersionCollection.groovy

+     * Uses basic logic about our releases to determine if this version has been previously released
+     */
+    private boolean isReleased(Version version) {
+        return version.revision > 0 || (version.revision > 1 && currentVersion.equals(Version.fromString("5.1.2")))


Do we actually need this hack? None of the existing logic should be thinking any 5.x branch is current right? I understand it matters from the theoretical side, but in master 5.x version constants shouldn't even exist?

++. I kept it in there just due to some initial notes I had taken about what released meant.

rjernst · 2018-02-08T16:21:42Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/VersionCollection.groovy

     */
-    Version getBWCSnapshotForCurrentMajor() {
-        return getLastSnapshotWithMajor(currentVersion.major)
+    List<Version> getVersionsWireCompatibleWithCurrent() {


I find these longer names actually harder to read. Can we go back to the old names (circa mid last year) so we could have eg bwcVersions.indexCompatible and bwcVersions.wireCompatible? Ie getIndexCompatible() and getWireCompatible() methods here.

rjernst · 2018-02-08T16:22:31Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/VersionCollection.groovy

-        Version version = getLastSnapshotWithMajor(currentVersion.major - 1)
-        assert version != null : "getBWCSnapshotForPreviousMajor(): found no versions in the previous major"
-        return version
+    List<Version> getSnapshotVersionsWireCompatibleWithCurrent() {


Similar to the request above, I think this should simply be getSnapshots()

rjernst · 2018-02-08T16:22:57Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/VersionCollection.groovy


-            prevConsideredVersion = currConsideredVersion
+        // re-add the currentVersion to the set
+        versionSet.add(currentVersion)


Why would current be in bwc versions at all?

discussed w/ you in person about the test failures, would love to fix up in another PR

rjernst · 2018-02-08T16:26:14Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/VersionCollection.groovy

+     */
+    private Version removeAndReaddAsSnapshot(Version version) {
+        versionSet.remove(version)
+        Version snapshotVersion = new Version(version.major, version.minor, version.revision, version.suffix, buildSnapshot)


I dont think this is correct. Whether we are building a snapshot should not affect whether we expect snapshots from unreleased old branches. Eg if we are running tests of a release version of 6.3.0, 5.6.7 does not suddenly become released; it is still a snapshot we must build.

I assume you are talking about the buildSnapshot being passed in? Id like to discuss that because not doing that breaks the VersionUtilsTests if you pass in the right mojo to simulate a release build. I did this to maintain the compat w/ the last change to the code. If its wrong logic, im ++ for fixing

rjernst · 2018-02-08T16:27:21Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/VersionCollection.groovy

+    /**
+     * Helper function for turning a version into a snapshot version, removing and readding it to the tree
+     */
+    private Version removeAndReaddAsSnapshot(Version version) {


Why not just replaceAsSnapshot?

This reverts commit 5996a17.

hub-cap · 2018-02-10T01:53:33Z

6.x has a failing test. Im going to be working on this shortly, but not tonight!

  1> [2018-02-09T22:54:27,061][INFO ][o.e.t.VersionUtilsTests  ] [testGradleVersionsMatchVersionUtils]: before test
  1> [2018-02-09T22:54:27,061][INFO ][o.e.t.VersionUtilsTests  ] Looked up versions [tests.gradle_index_compat_versions=5.0.0,5.0.1,5.0.2,5.1.1,5.1.2,5.2.0,5.2.1,5.2.2,5.3.0,5.3.1,5.3.2,5.3.3,5.4.0,5.4.1,5.4.2,5.4.3,5.5.0,5.5.1,5.5.2,5.5.3,5.6.0,5.6.1,5.6.2,5.6.3,5.6.4,5.6.5,5.6.6,5.6.7,5.6.8-SNAPSHOT,6.0.0,6.0.1,6.1.0,6.1.1,6.1.2,6.1.3,6.2.0,6.2.1,6.2.2-SNAPSHOT]
  1> [2018-02-09T22:54:27,063][INFO ][o.e.t.VersionUtilsTests  ] [testGradleVersionsMatchVersionUtils]: after test
  2> REPRODUCE WITH: ./gradlew :test:framework:test -Dtests.seed=1AF0A05FFF700D6C -Dtests.class=org.elasticsearch.test.VersionUtilsTests -Dtests.method="testGradleVersionsMatchVersionUtils" -Dtests.security.manager=true -Dtests.locale=sw-CD -Dtests.timezone=America/Cordoba
FAILURE 0.01s | VersionUtilsTests.testGradleVersionsMatchVersionUtils <<< FAILURES!
   > Throwable #1: java.lang.AssertionError: expected:<[5.0.0, 5.0.1, 5.0.2, 5.1.1, 5.1.2, 5.2.0, 5.2.1, 5.2.2, 5.3.0, 5.3.1, 5.3.2, 5.3.3, 5.4.0, 5.4.1, 5.4.2, 5.4.3, 5.5.0, 5.5.1, 5.5.2, 5.5.3, 5.6.0, 5.6.1, 5.6.2, 5.6.3, 5.6.4, 5.6.5, 5.6.6, 5.6.7, 5.6.8, 6.0.0, 6.0.1, 6.1.0, 6.1.1, 6.1.2, 6.1.3, 6.2.0, 6.2.1]> but was:<[5.0.0, 5.0.1, 5.0.2, 5.1.1, 5.1.2, 5.2.0, 5.2.1, 5.2.2, 5.3.0, 5.3.1, 5.3.2, 5.3.3, 5.4.0, 5.4.1, 5.4.2, 5.4.3, 5.5.0, 5.5.1, 5.5.2, 5.5.3, 5.6.0, 5.6.1, 5.6.2, 5.6.3, 5.6.4, 5.6.5, 5.6.6, 5.6.7, 6.0.0, 6.0.1, 6.1.0, 6.1.1, 6.1.2, 6.1.3, 6.2.0, 6.2.1]>
   >    at __randomizedtesting.SeedInfo.seed([1AF0A05FFF700D6C:CF33BA5A326040A2]:0)
   >    at org.elasticsearch.test.VersionUtilsTests.testGradleVersionsMatchVersionUtils(VersionUtilsTests.java:426)
   >    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
   >    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
   >    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
   >    at java.base/java.lang.reflect.Method.invoke(Method.java:564)
   >    at java.base/java.lang.Thread.run(Thread.java:844)
  1> [2018-02-09T22:54:27,072][INFO ][o.e.t.VersionUtilsTests  ] [testResolveReleasedVersionsForStableBranchBehindStableBranch]: before test
  1> [2018-02-09T22:54:27,073][INFO ][o.e.t.VersionUtilsTests  ] [testResolveReleasedVersionsForStableBranchBehindStableBranch]: after test
  1> [2018-02-09T22:54:27,076][INFO ][o.e.t.VersionUtilsTests  ] [testAllVersionsSorted]: before test
  1> [2018-02-09T22:54:27,076][INFO ][o.e.t.VersionUtilsTests  ] [testAllVersionsSorted]: after test
  1> [2018-02-09T22:54:27,078][INFO ][o.e.t.VersionUtilsTests  ] [testResolveReleasedVersionsAtVersionBumpIn6x]: before test
  1> [2018-02-09T22:54:27,080][INFO ][o.e.t.VersionUtilsTests  ] [testResolveReleasedVersionsAtVersionBumpIn6x]: after test

Generalizing BWC building so that there is less code to modify for a release. This ensures we do not need to think about what major or minor version is in the gradle code. It follows the general rules of the elastic release structure. For more information on the rules, see the VersionCollection's javadoc. This also removes the additional bwc snapshots that will never be released, such as 6.0.2, which were being built and tested against every time we ran bwc tests. Additionally, it creates 4 new projects that correspond to the different types of snapshots that may exist for a given version. Its possible to now run those individual tasks to work out bwc logic whereas previously it was impossible and the entire suite of bwc tests had to be run to work out any logic changes in the build tools' bwc project. Please note that if the project does not make sense for the version that is current, that an error will be thrown from that individual project if an attempt is made to run it. This should allow for automating the version bumps as well, since it removes all the hardcoded version logic from the configs.

Version Utils did not previously have logic that removed the last majors minor snapshot if there was a next bugfix and maintenance bugfix release. This adds the logic and fixes some broken assumptions in tests as well. relates elastic#28505

Version Utils did not previously have logic that removed the last majors minor snapshot if there was a next bugfix and maintenance bugfix release. This adds the logic and fixes some broken assumptions in tests as well. relates #28505

hub-cap added 6 commits February 1, 2018 21:07

BWC cleanup - wip

293529c

fixups

1ea56da

Adding a basic test, will add much more before refactoring that mess …

e6ce2f2

…i made

Adding much test for the gross that im about to refactor

52400d4

Mucho cleanup on aisle VersionCollection

633ddb5

various cleanup to minimize the drift between what was there and is now

340ab29

hub-cap requested review from DaveCTurner and rjernst February 2, 2018 22:13

hub-cap added v7.0.0 v6.3.0 labels Feb 2, 2018

hub-cap added 3 commits February 2, 2018 16:14

Minimize drift a bit more

7eefa6f

One more spacing issue

0e5f3de

Merge remote-tracking branch 'upstream/master' into bwc_changes

029aa86

DaveCTurner reviewed Feb 3, 2018

View reviewed changes

hub-cap added 2 commits February 5, 2018 08:11

Removed last treeset mention from the big logic bit of the vc

edab607

Cleanup as per review

14eae46

hub-cap added 6 commits February 5, 2018 12:51

Fixing the version tests to align back with the gradle expectations

6f3db18

flub in a prior code move, moving back

64a1fd5

Rename versionCollection to bwcVersions in build scripts

6e2cae4

Merge remote-tracking branch 'upstream/master' into bwc_changes

75b7b5b

Merge remote-tracking branch 'upstream/master' into bwc_changes

e4278ad

Fixing a broken assumption about the bwc subprojects

d640412

rjernst approved these changes Feb 8, 2018

View reviewed changes

hub-cap added 4 commits February 8, 2018 13:34

Merge remote-tracking branch 'upstream/master' into bwc_changes

8fc5296

Making the naming much better for a few things

27f942a

Merge remote-tracking branch 'upstream/master' into bwc_changes

ea4829c

Merge remote-tracking branch 'upstream/master' into bwc_changes

9ae9388

hub-cap added 3 commits February 9, 2018 12:42

Remove unused version snapshot

d3853f3

Remove another unneeded snapshot version

5996a17

Revert "Remove another unneeded snapshot version"

590d874

This reverts commit 5996a17.

hub-cap merged commit e0bea70 into elastic:master Feb 9, 2018

hub-cap deleted the bwc_changes branch February 9, 2018 20:55

hub-cap added the backport pending label Feb 10, 2018

clintongormley added the :Delivery/Build Build or test infrastructure label Feb 12, 2018

hub-cap removed the backport pending label Feb 12, 2018

hub-cap mentioned this pull request Feb 12, 2018

Add released major logic to version utils #28644

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bwc changes #28505

Bwc changes #28505

hub-cap commented Feb 2, 2018

DaveCTurner left a comment

DaveCTurner Feb 3, 2018

hub-cap Feb 3, 2018

DaveCTurner Feb 3, 2018

hub-cap Feb 3, 2018

DaveCTurner Feb 3, 2018

hub-cap Feb 3, 2018

hub-cap Feb 5, 2018

DaveCTurner Feb 3, 2018

hub-cap Feb 3, 2018

DaveCTurner Feb 3, 2018

hub-cap Feb 3, 2018

DaveCTurner Feb 3, 2018

DaveCTurner Feb 3, 2018

hub-cap Feb 3, 2018

DaveCTurner Feb 3, 2018

hub-cap Feb 3, 2018

rjernst Feb 8, 2018

hub-cap commented Feb 3, 2018

hub-cap commented Feb 3, 2018

hub-cap commented Feb 5, 2018

rjernst left a comment

rjernst Feb 8, 2018

hub-cap Feb 8, 2018

rjernst Feb 8, 2018

rjernst Feb 8, 2018

rjernst Feb 8, 2018

hub-cap Feb 8, 2018

rjernst Feb 8, 2018

hub-cap Feb 8, 2018

rjernst Feb 8, 2018

hub-cap commented Feb 10, 2018 •

edited

Loading

		@@ -0,0 +1,226 @@
		package org.elasticsearch.gradle

		class VersionCollectionTest extends GroovyTestCase {

Bwc changes #28505

Bwc changes #28505

Conversation

hub-cap commented Feb 2, 2018

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap commented Feb 3, 2018

hub-cap commented Feb 3, 2018

hub-cap commented Feb 5, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap commented Feb 10, 2018 • edited Loading

hub-cap commented Feb 10, 2018 •

edited

Loading