New versioning rules prep #740
Conversation
changed a while ago to EntitySpecResolver; removes references to catalog items and hokey lookup
expand tests. some obscure ordering items are different now.
… tests, and usages
and bundle finder supports either, and does the conversion for you
|
LGTM, but a bit nervous about not relying on |
|
Couldn't find where we sanitise the catalog items ID to be used for persistence file names. If changing the naming rules for the IDs then perhaps changing the sanitising rules would be needed as well? |
geomacy
left a comment
geomacy
left a comment
There was a problem hiding this comment.
hi Alex some observations and thoughts below. I've some concerns around the version comparison which seems to go beyond what we discussed on the mailing list, that may just be a difference in interpretation but I do think there are some valid issues to think about here. Let me know what you think.
|
|
||
| } else if ((Character.isDigit(ca) || nza>0) && (Character.isDigit(cb) || nzb>0)) { | ||
| // both sides are numbers, but at least one is a sequence of zeros | ||
| if (nza==0) { |
There was a problem hiding this comment.
Not part of this PR I know, but the code at 189:
if (ca == 0 && cb == 0) {
// The strings compare the same. Perhaps the caller
// will want to call strcmp to break the tie.
return nza - nzb;
}
is a hangover from the code's origin in a C implementation (zero character to terminate a string). It doesn't make sense here in Java.
There was a problem hiding this comment.
his charAt returns 0 if we've gone beyond the string length, i think it might rely on that
| if ((result = compareRight(a.substring(ia), b.substring(ib))) != 0) { | ||
| return result; | ||
| } | ||
| // numeric portion is the same; previously we incremented and checked again |
There was a problem hiding this comment.
This is complicated, especially the recursive comparison after comparing matching digit sequences. Why are these changes necessary for this PR? (Apart from the efficiency saving of not incrementing and rechecking).
I don't think we need this anyway, see comments below on VersionComparator.
There was a problem hiding this comment.
i noticed some behaviour which i thought was undesired behaviour of NaturalOrderComparator, as indicated by the new tests
| } | ||
| return false; | ||
| private String versionWithQualifier(String v1) { | ||
| Matcher q = Pattern.compile("([0-9]+(\\.[0-9]+(\\.[0-9])?)?)(.*)").matcher(v1); |
There was a problem hiding this comment.
This is missing a +, just after the ^ below
Matcher q = Pattern.compile("([0-9]+(\\.[0-9]+(\\.[0-9])?)?)(.*)").matcher(v1);
^
Why not format this like the similar regexes in BrooklynVersionSyntax, e.g. VALID_OSGI_VERSION_REGEX.
| return true; | ||
| } | ||
| return false; | ||
| private String versionWithQualifier(String v1) { |
There was a problem hiding this comment.
versionWithoutQualifier, rather.
|
|
||
| String u1 = versionWithQualifier(v1); | ||
| String u2 = versionWithQualifier(v2); | ||
| int uq = NaturalOrderComparator.INSTANCE.compare(u1, u2); |
There was a problem hiding this comment.
This gives .compare("1.1", "1.01") as different, but they should be the same.
There was a problem hiding this comment.
i think it's important for this comparator not to return 0 unless strings are truly equal; otherwise we risk indeterminate ordering for things which other routines consider different.
therefore in perverse cases we return a consistent ordering, even if it seems a little arbitrary.
in this case, if someone did perversely use 1.01, 1.1, 1.09, 1.9, and 1.10 as versions, i think that order -- which is what we return -- is the least surprising.
There was a problem hiding this comment.
But it's surely not correct for Versions - I agree NaturalOrderComparator should return 0 only if the Strings are truly equal; I don't think it's the right thing to use for the major/minor/patch - these are meant to be compared numerically.
| parts.addAll(Arrays.asList(remaining.split("(?<=[^0-9\\p{L}])|(?=[^0-9\\p{L}])"))); | ||
| return parts.toArray(new String[parts.size()]); | ||
| // finally, do normal comparison if both have qualifier or both unqualified (in case leading 0's are used) | ||
| return NaturalOrderComparator.INSTANCE.compare(v1, v2); |
There was a problem hiding this comment.
I don't see why we should have to call compare again here, by this stage we should have decided how they compare based on the rules discussed above.
There was a problem hiding this comment.
i think this block handles the case of 01 v 0 and possibly separators -- in any case useful to have a fallback (not return 0 unless it's the same string)
| * treating anything with a qualifier lower than the corresponding without but higher than items before | ||
| * (1.2 > 1.2-rc > 1.1), | ||
| * SNAPSHOT items always lowest rated (1.0 > 2-SNAPSHOT), and | ||
| * qualifier sorting follows {@link NaturalOrderComparator} (1-M10 > 1-M9). |
|
|
||
| private static String toGoodVersion(String input, String qualifierSeparator, boolean requireMinorAndPatch) { | ||
| Preconditions.checkNotNull(input); | ||
| final String FUZZY_REGEX = |
There was a problem hiding this comment.
Might be nice to try to get all the regexes that define all these variants into one place, with duplications removed.
There was a problem hiding this comment.
most are; the few that aren't i think either aren't worth it or are special to how they are used; moving them elsewhere risks things breaking if e.g. the grouping rules are changed
| /** True if the argument matches the Brooklyn version syntax, | ||
| * <code>MAJOR.MINOR.POINT-QUALIFIER</code> or part thereof (not ending in a period though), | ||
| * where the first three are whole numbers and the final piece is any valid OSGi token | ||
| * (something satisfying {@link #isGoodTypeName(String)} with no periods). |
There was a problem hiding this comment.
isGoodTypeName no longer exists; here and below
| * <code>MAJOR.MINOR.POINT.QUALIFIER</code> or part thereof (not ending in a period though), | ||
| * where the first three are whole numbers and the final piece is any valid OSGi token | ||
| * (something satisfying {@link #isGoodTypeName(String)} with no periods). | ||
| * See also {@link #isGoodVersion(String)}. |
There was a problem hiding this comment.
isGoodVersion no longer exists
|
i've cleaned up several things, but multiple discussion threads remain:
for recommended versions it is the same, isn't it? that's the important thing. for other things yes it will be surprising but someone making an OSGi bundle relying on that converter's obscure behaviour is asking for trouble. i'd rather optimize to be sensible for people doing foolish non-recommended things in non-maven / non-OSGi cases, where version
again only an issue if using non-recommended IDs, and we don't rely on those files very much now (esp once YAML BOMs are added as bundles), so content to leave this as a bug.
semver says qualifer makes something less than unqualified, e.g. our ordering is different to both; we should make this clear in the docs but i think that's better than forcing people to understand the surprising ordering in osgi. the one place this could be an issue is if we've relied on OSGi to resolve version ranges. but i think let's cross that bridge when we come to it, rather than have
discussed above, but captured here for completeness |
ahgittin
force-pushed
the
version-new-rules-prep
branch
from
28c1b5c to
ee853f5
Compare
That's everyone using maven. When the bundle's version is different from the catalog.bom's derived OSGi version Brooklyn will deny loading the file, right? If so we should just say that we are not supporting non-standard versions (and not accept them from the start) instead of documenting the format as deprecated, but valid and letting the user find out if it works or not for them. Wdyt? |
|
I don't think there's any harm in having a slightly stricter validation in place, in fact as a user I would find that more helpful than having Brooklyn adapt to me doing things badly. Should we take the discussion of the above points back on to the mailing list or just deal with them here? |
|
@neykov do we know anyone actually using maven to make brooklyn bundles with a version syntax that will break it? i think it's far enough below the radar i'm not concerned about it. we should log warns on non-recommended versions (which i think we do, probably too much) but disallowing them now is going to break backwards compatibility. also it's not uncommon that people use OSGi versions directly in maven which will work fine in Brooklyn even though it's not the recommended syntax. feels like it might be antisocial to prevent that. in short current approach -- have a recommended syntax, and warn but make best effort if people not using it -- is best for now, and we can become stricter in the next version. |
4 participants
Creates
BrooklynVersionSyntaxto assist with brooklyn/osgi versioning. Includes conversion routines, many tests, and many usages updated.Versions are handled according to apache/brooklyn-docs#198 -- but we're not yet applying them in the code -- so the problem discussed at #672 still stands -- but should be easy to fix using the groundwork here and in #737.
Builds on #737 which laid the foundation but didn't recognise the need for a specific recommended brooklyn version syntax (this includes and closes that in case it's easier to review and merge just this, ignoring that).