Make the build version qualifier aware #32128
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,35 +16,43 @@ public final class Version implements Comparable<Version> { | |
| /** | ||
| * Suffix on the version name. | ||
| */ | ||
| private final String qualifier; | ||
|
|
||
| private static final Pattern pattern = | ||
| Pattern.compile("(\\d)+\\.(\\d+)\\.(\\d+)(-alpha\\d+|-beta\\d+|-rc\\d+)?(-SNAPSHOT)?"); | ||
|
|
||
| Version(int major, int minor, int revision, String qualifier, boolean snapshot) { | ||
| Objects.requireNonNull(major, "major version can't be null"); | ||
| Objects.requireNonNull(minor, "minor version can't be null"); | ||
| Objects.requireNonNull(revision, "revision version can't be null"); | ||
| this.major = major; | ||
| this.minor = minor; | ||
| this.revision = revision; | ||
| this.snapshot = snapshot; | ||
| if (qualifier == null || qualifier.isEmpty()) { | ||
| this.qualifier = ""; | ||
| } else if (qualifier.startsWith("-")) { | ||
| this.qualifier = qualifier; | ||
| } else { | ||
| this.qualifier = "-" + qualifier; | ||
|
There was a problem hiding this comment. Do we need to do the formatting here (prefixing with dash), or could this be done when printed? There was a problem hiding this comment. We used to store it with a dash so existing code expected it, but I didn't want to add have it in the property, so this takes care of that. I think the cleanest way would be to have an enum for the qualifier. There was a problem hiding this comment. An enum won't work since these are numbered (or it would require separate variable to keep track of the number). But I don't see why the leniency needs to exist here to add the dash if it doesn't exist. I think the qualifier should always have no dash internally, just like we don't have dot prefixes on the numeric parts of the version. |
||
| } | ||
|
|
||
| int suffixOffset = 0; | ||
| if (this.qualifier.isEmpty()) { | ||
| // no qualifier will be considered smaller, uncomment to change that | ||
| // suffixOffset = 100; | ||
|
There was a problem hiding this comment. What is the purpose of having this commented out conditional block? With the new rules, we shouldn't ever be comparing a qualified build against each other or a non qualified build, since qualified and non qualified of the same version have the exact same bwc behavior. There was a problem hiding this comment. I added this comment some time ago when I discovered that There was a problem hiding this comment.
There shouldn't be anything needing to choose a beta over an alpha? There should be nothing using any qualified build to check bwc. There was a problem hiding this comment. I removed the commented code to avoid confusion. There was a problem hiding this comment. I don't know what you mean by "consider qualifier in the build". There should be no attempt to test bwc of any qualified version, so I don't believe the build needs to know about it in our build Version, since that class is all about which versions we bwc test against. There was a problem hiding this comment. The version class is now used more broadly, including in figuring out dependencies ( as it's string value is set as project.version which is then used when considering dependencies ). I don't think we should remove the qualifier from the version right now, we should eventually better express requirements for a version used in bwc, perhaps as part of #32872. There was a problem hiding this comment.
There shouldn't be any dependencies on a qualified version, so there should be no need to serialize it into a string value.
Why would we keep something around that is unused? I think it only adds to confusion in a class that is already difficult to under (our gradle's Version class). |
||
| } else { | ||
| if (this.qualifier.contains("alpha")) { | ||
| suffixOffset += parseSuffixNumber(this.qualifier.substring(6)); | ||
| } else if (this.qualifier.contains("beta")) { | ||
| suffixOffset += 25 + parseSuffixNumber(this.qualifier.substring(5)); | ||
| } else if (this.qualifier.contains("rc")) { | ||
| suffixOffset += 50 + parseSuffixNumber(this.qualifier.substring(3)); | ||
| } | ||
| else { | ||
| throw new IllegalArgumentException( | ||
| "Suffix must contain one of: alpha, beta or rc instead of: " + this.qualifier | ||
| ); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -54,7 +62,7 @@ public Version(int major, int minor, int revision, String suffix, boolean snapsh | |
|
|
||
| private static int parseSuffixNumber(String substring) { | ||
| if (substring.isEmpty()) { | ||
| throw new IllegalArgumentException("Invalid qualifier, must contain a number e.x. alpha2"); | ||
| } | ||
| return Integer.parseInt(substring); | ||
| } | ||
|
|
@@ -81,7 +89,7 @@ public static Version fromString(final String s) { | |
| public String toString() { | ||
| final String snapshotStr = snapshot ? "-SNAPSHOT" : ""; | ||
| return String.valueOf(getMajor()) + "." + String.valueOf(getMinor()) + "." + String.valueOf(getRevision()) + | ||
| (qualifier == null ? "" : qualifier) + snapshotStr; | ||
| } | ||
|
|
||
| public boolean before(Version compareTo) { | ||
|
|
@@ -121,12 +129,12 @@ public boolean onOrBeforeIncludingSuffix(Version otherVersion) { | |
| return id < otherVersion.getId(); | ||
| } | ||
|
|
||
| if (qualifier.equals("")) { | ||
| return otherVersion.getQualifier().equals(""); | ||
| } | ||
|
|
||
|
|
||
| return otherVersion.getQualifier().equals("") || qualifier.compareTo(otherVersion.getQualifier()) < 0; | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -139,13 +147,27 @@ public boolean equals(Object o) { | |
| revision == version.revision && | ||
| id == version.id && | ||
| snapshot == version.snapshot && | ||
| Objects.equals(qualifier, version.qualifier); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
|
|
||
| return Objects.hash(major, minor, revision, id, snapshot, qualifier); | ||
| } | ||
|
|
||
| public boolean isSameVersionNumber(Version other) { | ||
| return major == other.major && | ||
| minor == other.minor && | ||
| revision == other.revision; | ||
| } | ||
|
|
||
| public Version withoutQualifier() { | ||
| return new Version(major, minor, revision, null, snapshot); | ||
| } | ||
|
|
||
| public Version withoutSnapshot() { | ||
| return new Version(major, minor, revision, qualifier, false); | ||
| } | ||
|
|
||
| public int getMajor() { | ||
|
|
@@ -168,8 +190,8 @@ public boolean isSnapshot() { | |
| return snapshot; | ||
| } | ||
|
|
||
| public String getQualifier() { | ||
| return qualifier; | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,11 +27,29 @@ public static Map<String, String> getVersions() { | |
| private static final Map<String, String> versions = new HashMap<String, String>(); | ||
| static { | ||
| Properties props = getVersionProperties(); | ||
| Version baseVersion = Version.fromString(props.getProperty("elasticsearch")); | ||
| if (baseVersion.isSnapshot()) { | ||
| throw new IllegalArgumentException("version.properties can't contain a snapshot version for elasticsearch. " + | ||
| "Use the `build.snapshot` property instead."); | ||
| } | ||
| if (baseVersion.getQualifier().isEmpty() == false) { | ||
| throw new IllegalArgumentException("version.properties can't contain a version qualifier for elasticsearch." + | ||
| "Use the `build.version_qualifier` property instead."); | ||
| } | ||
| elasticsearch = new Version( | ||
| baseVersion.getMajor(), | ||
| baseVersion.getMinor(), | ||
| baseVersion.getRevision(), | ||
| // TODO: Change default to "" after CI "warm-up" by adding -Dbuild.version_qualifier="" to ML jobs to produce | ||
| // a snapshot. | ||
|
There was a problem hiding this comment. We now have the ability to set the ML C++ version qualifier when kicking off a build, so if it makes life easier for you I can create a one-off build of ML C++ 7.0.0-SNAPSHOT artifacts now so that the PR build can pass with a default qualifier of |
||
| System.getProperty("build.version_qualifier", "alpha1"), | ||
|
There was a problem hiding this comment. This should not default to There was a problem hiding this comment. I think that this also means that we need to percolate this through to CI otherwise the builds will be looking for ML 7.0.0-SNAPSHOT artifacts which will not exist. There was a problem hiding this comment. One reason that I do not like the default of
There was a problem hiding this comment. I think the confusion is that without any qualifier, we'll be producing An alternative is to continue having the qualifier in There was a problem hiding this comment. I am starting to come around to the view that we simply always produce unqualified builds, yet from time to time we will ship a qualified build with a progression of There was a problem hiding this comment. @jasontedor you mean to have CI always build a snapshot without a qualifier like I like it how that simplifies things, and I think it is ok since we either don't consider the qualifiers already ( e.x. we remove qualified versions from bwc tests ) or we are making changes to compare without the version qualifier ( e.x. when comparing build with |
||
| Boolean.parseBoolean(System.getProperty("build.snapshot", "true")) | ||
| ); | ||
| lucene = props.getProperty("lucene"); | ||
| for (String property : props.stringPropertyNames()) { | ||
| versions.put(property, props.getProperty(property)); | ||
| } | ||
| versions.put("elasticsearch", elasticsearch.toString()); | ||
| } | ||
|
|
||
| private static Properties getVersionProperties() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this leniency? The semantics should be either no qualifier is null, or empty string, but not either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's because the instance was constructed with null most of the time but, we mostly get the qualifier to compare against it, so having
""instead is more convenient and readable.We could disallow nulls in the constructor, I would clean this up as part of #32872