-
Notifications
You must be signed in to change notification settings - Fork 24.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the build version qualifier aware #32128
Changes from 24 commits
2bb8616
9dd442c
9380817
08096d2
018fafa
76b67b3
ee2359f
54176b6
67c06f3
cbaa33a
6071cfd
218bbf4
d5ceca9
6507480
73d7496
4c68d70
ad58c10
63910ab
de4b304
13d92a5
75489c7
64a833d
e10f818
1fe9ac4
051539d
c9be34e
4195a55
377b632
dd5122e
d5a3f2e
6a47804
53b1fce
85d010e
6f53bd3
4074bc3
e76a255
2e7b567
dcf67a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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 suffix; | ||
private final String qualifier; | ||
|
||
private static final Pattern pattern = | ||
Pattern.compile("(\\d)+\\.(\\d+)\\.(\\d+)(-alpha\\d+|-beta\\d+|-rc\\d+)?(-SNAPSHOT)?"); | ||
|
||
public Version(int major, int minor, int revision, String suffix, boolean 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; | ||
this.suffix = suffix == null ? "" : suffix; | ||
if (qualifier == null || qualifier.isEmpty()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
this.qualifier = ""; | ||
} else if (qualifier.startsWith("-")) { | ||
this.qualifier = qualifier; | ||
} else { | ||
this.qualifier = "-" + qualifier; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.suffix.isEmpty()) { | ||
// no suffix will be considered smaller, uncomment to change that | ||
if (this.qualifier.isEmpty()) { | ||
// no qualifier will be considered smaller, uncomment to change that | ||
// suffixOffset = 100; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this comment some time ago when I discovered that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed the commented code to avoid confusion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.suffix.contains("alpha")) { | ||
suffixOffset += parseSuffixNumber(this.suffix.substring(6)); | ||
} else if (this.suffix.contains("beta")) { | ||
suffixOffset += 25 + parseSuffixNumber(this.suffix.substring(5)); | ||
} else if (this.suffix.contains("rc")) { | ||
suffixOffset += 50 + parseSuffixNumber(this.suffix.substring(3)); | ||
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"); | ||
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 suffix, must contain a number e.x. alpha2"); | ||
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()) + | ||
(suffix == null ? "" : suffix) + snapshotStr; | ||
(qualifier == null ? "" : qualifier) + snapshotStr; | ||
} | ||
|
||
public boolean before(Version compareTo) { | ||
|
@@ -121,12 +129,12 @@ public boolean onOrBeforeIncludingSuffix(Version otherVersion) { | |
return id < otherVersion.getId(); | ||
} | ||
|
||
if (suffix.equals("")) { | ||
return otherVersion.getSuffix().equals(""); | ||
if (qualifier.equals("")) { | ||
return otherVersion.getQualifier().equals(""); | ||
} | ||
|
||
|
||
return otherVersion.getSuffix().equals("") || suffix.compareTo(otherVersion.getSuffix()) < 0; | ||
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(suffix, version.suffix); | ||
Objects.equals(qualifier, version.qualifier); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
|
||
return Objects.hash(major, minor, revision, id, snapshot, suffix); | ||
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 dropQualifier() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we use the "without" terminology? I think this better matches other code like the builders for java time stuff having eg |
||
return new Version(major, minor, revision, "", snapshot); | ||
} | ||
|
||
public Version dropSnapshot() { | ||
return new Version(major, minor, revision, qualifier, false); | ||
} | ||
|
||
public int getMajor() { | ||
|
@@ -168,8 +190,8 @@ public boolean isSnapshot() { | |
return snapshot; | ||
} | ||
|
||
public String getSuffix() { | ||
return suffix; | ||
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(); | ||
elasticsearch = Version.fromString(props.getProperty("elasticsearch")); | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should not default to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One reason that I do not like the default of
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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.
nit: qualifier'd -> qualified
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.
fixed, thanks