-
Notifications
You must be signed in to change notification settings - Fork 121
Quick fix to add available matching version for Imported packages #1962
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
base: master
Are you sure you want to change the base?
Quick fix to add available matching version for Imported packages #1962
Conversation
|
@HannesWell Corrected: |
cf7ab4f to
6e79a47
Compare
de3e456 to
971407a
Compare
f6a6a98 to
adb0579
Compare
| exportedPackage.getVersion().toString(), NameVersionDescriptor.TYPE_PACKAGE); | ||
| exportedPackage.getExporter().getBundle(); | ||
|
|
||
| if (("java".equals(name) || name.startsWith("java."))) { //$NON-NLS-1$ //$NON-NLS-2$ |
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.
can this check be moved to the beginning of the loop, as soon as name is available?
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.
done
adb0579 to
7089fa0
Compare
7089fa0 to
7687e6f
Compare
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
HannesWell
left a comment
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.
Just one point from me at the moment, I'll try to do a full review this evening.
| public static String computeInitialRequirementVersionRange(String version) { | ||
| if (version != null && VersionUtil.validateVersion(version).isOK()) { | ||
| Version pvi = Version.parseVersion(version); | ||
| return new VersionRange( | ||
| "[" + pvi.getMajor() + "." + pvi.getMinor() + "," + (pvi.getMajor() + 1) + ")") //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$//$NON-NLS-4$ | ||
| .toString(); | ||
| } | ||
| return ""; //$NON-NLS-1$ | ||
| } |
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.
Recently I added a method that does this to ManifestUtils:
eclipse.pde/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/ManifestUtils.java
Lines 402 to 410 in 6a28c28
| public static Optional<VersionRange> createConsumerRequirementRange(Version version) { | |
| if (version != null && !Version.emptyVersion.equals(version)) { | |
| return Optional.ofNullable(new VersionRange(VersionRange.LEFT_CLOSED, // | |
| new Version(version.getMajor(), version.getMinor(), 0), // | |
| new Version(version.getMajor() + 1, 0, 0), // | |
| VersionRange.RIGHT_OPEN)); | |
| } | |
| return Optional.empty(); | |
| } |
Admittedly this method would probably better be moved to this class and probably be used instead of the existing method. That would hopefully add proper version ranges in more places.
I can try to have a look at unifying these as soon as possible. Or if you want to have a look at it, it would be more than welcome.
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.
@HannesWell
I have moved your method createConsumerRequirementRange to VersionUtil class and updated the corresponding callers(found 2 classes - JavaResolutionFactory and ImportPackageObject).
Kindly check the changes.
35827e3 to
cc2fb8c
Compare
This is to provide a quick fix for adding the available matching version for Imported packages in MANIFEST.MF warning.
Similar to the fix: #1623 and as suggested in here
Attaching screenshot for the quick fix:
After applying: