Skip to content
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

feat: use Semver compatibility for dependency info #101

Merged
merged 2 commits into from
Apr 22, 2021

Conversation

keturn
Copy link
Member

@keturn keturn commented Apr 20, 2021

Version 7 introduced the use of the Semver library (#80) for some functions of version handling. It changed the behavior of getNextVersion methods (as mentioned in its PR), and that left DependencyInfo returning an unsatisfiable version range when a module's minVersion targets a snapshot. (#100)

public Version getMaxVersion() {
if (maxVersion == null) {
if (minVersion.getMajor() == 0) {
return minVersion.getNextMinorVersion();
}
return minVersion.getNextMajorVersion();

Rather than try to re-implement the semantics of satisfying semantic versioning in the DependencyInfo and VersionRange classes, I propose using more of the Semver library's methods for this.

This PR is a step in that direction. It keeps the VersionRange class intact to preserve compatibility, but if a DependencyInfo was defined with only a minimal version, instead of building a gestalt.VersionRange for it, we compile a Semver expression that will determine what matches.

Dependencies: #98. If that is not merged yet, you can review using this diff: v7/test/java11...v7/feat/semverComparison

Questions for Review

  • Is the caret expression the right one?
  • Is it worth keeping the existing VersionRange class at all, or should we convert entirely?
    • If we convert, be mindful of VersionRangeTest. lowerSnapshotInRange and higherSnapshotOutOfRange are the ones that are easy to miss.
    • I only found one place in Terasology that directly uses VersionRange, and it's easy to update.
  • Instead of building an expression out of minVersion, should we allow module metadata to define its own expression?
    • We would need to use the Maven-style expressions in that case in order for compile-time dependency resolution to still work.

@keturn keturn added Topic: Module Requests, Issues and Changes related to gestalt modules v7 labels Apr 20, 2021
@keturn keturn requested a review from skaldarnar April 20, 2021 22:46
@keturn keturn added this to In progress in gestalt v7 migration (module & asset) via automation Apr 20, 2021
@keturn keturn marked this pull request as ready for review April 20, 2021 22:47

public SemverExpression(String source) {
this.source = source;
this.expression = ExpressionParser.newInstance().parse(source);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: error handling
or maybe using a documented exception here, and error handling in DependencyInfo?

@keturn keturn added the Type: Bug Issues reporting and PRs fixing problems label Apr 20, 2021
@keturn
Copy link
Member Author

keturn commented Apr 20, 2021

@@ -125,7 +112,7 @@ private void populateConstraints() {
Module versionedModule = registry.getModule(name, version.getVersion().get());
DependencyInfo info = versionedModule.getMetadata().getDependencyInfo(dependency);
if (info != null) {
constraintTable.put(version.getVersion().get(), new CompatibleVersions(info.versionRange(), info.isOptional() && !optionalStrategy.isRequired()));
constraintTable.put(version.getVersion().get(), new CompatibleVersions(info.versionPredicate(), info.isOptional() && !optionalStrategy.isRequired()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to have a uniform code formatter...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what prompted that thought on this line in particular?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, probably the length of that line 🤔

@skaldarnar
Copy link
Member

I leave the two open questions for later. I do believe that following any common way to define versions would be a reasonable choice (be it similar to npm's advanced range syntax or resembling gradle's version declarations).

@skaldarnar skaldarnar merged commit bd593ed into release/v7.x Apr 22, 2021
gestalt v7 migration (module & asset) automation moved this from In progress to Done Apr 22, 2021
@skaldarnar skaldarnar deleted the v7/feat/semverComparison branch April 22, 2021 21:11
@Cervator Cervator added this to the v7.1.0 milestone Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Topic: Module Requests, Issues and Changes related to gestalt modules Type: Bug Issues reporting and PRs fixing problems
Development

Successfully merging this pull request may close these issues.

None yet

3 participants