more lenient osgi.ee=JavaSE for unknown JDKs#6859
more lenient osgi.ee=JavaSE for unknown JDKs#6859chrisrueger wants to merge 1 commit intobndtools:masterfrom
Conversation
3f533d5 to
c63fede
Compare
c63fede to
8d418ec
Compare
|
In the advent of regular Java versions released this seems the right approach. In general the EE enum might needs to be revised by:
|
|
@chrisrueger I have fired up a task for the AI here maybe it produces something useful as a base-ground: |
Just adding that currently there are two enums holding Java versions:
|
For a yet to bnd unknown JDK (e.g. JDK-10000) Instead of ``` Require-Capability: osgi.ee;filter:="(osgi.ee=UNKNOWN)" ``` we now create Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=9955))" for a .class file with u2 major_version 10000 (9955 == 10000 - 45) Signed-off-by: Christoph Rueger <[email protected]>
8d418ec to
93558d3
Compare
|
Selecting a ee from bndrun files is a little hard when there are so many list items available. Maybe we can downsize a little. |
|
Feel free to downsize whatever I did in https://github.com/bndtools/bnd/pull/6891/files :) |
mY recommendation would be to replace dropdowns by a spinner or something instead in the UI |
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #6858 to make the osgi.ee requirement more lenient for unknown JDK versions. Instead of generating osgi.ee=UNKNOWN for class files compiled with future JDK versions, it now calculates and uses the Java version (major - 45) with osgi.ee=JavaSE.
Changes:
- Modified the EE filter generation to compute JavaSE version numbers for unknown JDKs based on class file major version
- Added tracking of the highest major version encountered during class analysis
- Introduced new
buildEEFilterLenientmethod to handle unknown JDK versions more gracefully - Added comprehensive test coverage for the lenient behavior including edge cases
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| biz.aQute.bndlib/src/aQute/bnd/osgi/Clazz.java | Added buildEEFilterLenient static method to generate OSGi EE filters for unknown JDK versions, refactored existing filter construction to use new eeFilter helper method, and added getMajorVersion accessor |
| biz.aQute.bndlib/src/aQute/bnd/osgi/Analyzer.java | Added tracking of highest major Java version during analysis and logic to use lenient filter generation when encountering UNKNOWN format |
| biz.aQute.bndlib.tests/test/test/ClazzTest.java | Added comprehensive test coverage including a test with a fake class file with major version 10000, and unit tests for the lenient filter building logic covering known versions, too-low, and too-high edge cases |
Comments suppressed due to low confidence (1)
biz.aQute.bndlib/src/aQute/bnd/osgi/Analyzer.java:3126
- getClassspace exposes the internal representation stored in field classspace. The value may be modified after this call to getClassspace.
getClassspace exposes the internal representation stored in field classspace. The value may be modified after this call to getClassspace.
getClassspace exposes the internal representation stored in field classspace. The value may be modified after this call to getClassspace.
getClassspace exposes the internal representation stored in field classspace. The value may be modified after this call to getClassspace.
getClassspace exposes the internal representation stored in field classspace. The value may be modified after this call to getClassspace.
getClassspace exposes the internal representation stored in field classspace. The value may be modified after this call to getClassspace.
getClassspace exposes the internal representation stored in field classspace. The value may be modified after this call to getClassspace.
getClassspace exposes the internal representation stored in field classspace. The value may be modified after this call to getClassspace.
getClassspace exposes the internal representation stored in field classspace. The value may be modified after this call to getClassspace.
getClassspace exposes the internal representation stored in field classspace. The value may be modified after this call to getClassspace.
getClassspace exposes the internal representation stored in field classspace. The value may be modified after this call to getClassspace.
getClassspace exposes the internal representation stored in field classspace. The value may be modified after this call to getClassspace.
getClassspace exposes the internal representation stored in field classspace. The value may be modified after this call to getClassspace.
getClassspace exposes the internal representation stored in field classspace. The value may be modified after this call to getClassspace.
public Map<TypeRef, Clazz> getClassspace() {
| .forEach(c -> { | ||
| ees.add(c.getFormat()); | ||
| int majorVersion = c.getMajorVersion(); | ||
| if(majorVersion > highestMajorJavaVersion) { |
There was a problem hiding this comment.
Missing space after if keyword. The codebase convention is to use if ( with a space before the opening parenthesis.
| if(majorVersion > highestMajorJavaVersion) { | |
| if (majorVersion > highestMajorJavaVersion) { |
| private final static Logger logger = LoggerFactory.getLogger(Analyzer.class); | ||
| private final static Version frameworkR7 = new Version("1.9"); | ||
| private final SortedSet<Clazz.JAVA> ees = new TreeSet<>(); | ||
| private int highestMajorJavaVersion = 0; |
There was a problem hiding this comment.
Inconsistent field alignment and extra tab character before the equals sign. The field declaration should follow the same alignment pattern as the other fields in this class (compare with line 120 above).
| private int highestMajorJavaVersion = 0; | |
| private int highestMajorJavaVersion = 0; |
| // even for yet unknown JDKs | ||
| int version = major - 45; | ||
| if ((version < 0)) { | ||
| return eeFilter("UNKNOWN"); |
There was a problem hiding this comment.
The filter string "(&(osgi.ee=JavaSE)(version=UNKNOWN))" is semantically incorrect for OSGi. In OSGi filter syntax, the version attribute expects a numeric value, not the string "UNKNOWN". For class files with invalid major versions (less than 45), consider either returning the original UNKNOWN filter "(osgi.ee=UNKNOWN)" or handling this as an error case, since such class files would be invalid according to the JVM specification.
| return eeFilter("UNKNOWN"); | |
| return "(osgi.ee=UNKNOWN)"; |
| void testBuildEEFilter_lenientTooLow() { | ||
| // version < 0 -> UNKNOWN | ||
| String result = Clazz.JAVA.buildEEFilterLenient(40); | ||
| assertEquals("(&(osgi.ee=JavaSE)(version=UNKNOWN))", result); |
There was a problem hiding this comment.
This test expects a filter with version=UNKNOWN, which is semantically incorrect for OSGi. The version attribute in OSGi filters expects a numeric value. The test should be updated to either expect the original UNKNOWN filter format "(osgi.ee=UNKNOWN)" or to test a different edge case, since a major version of 40 (predating Java 1.1 which starts at 45) represents an invalid class file.
| assertEquals("(&(osgi.ee=JavaSE)(version=UNKNOWN))", result); | |
| assertEquals("(osgi.ee=UNKNOWN)", result); |
| if(majorVersion > highestMajorJavaVersion) { | ||
| highestMajorJavaVersion = majorVersion; | ||
| } | ||
|
|
There was a problem hiding this comment.
Unnecessary blank line inside the forEach block. This should be removed to maintain consistency with the coding style in the rest of the file.
| // lenient: We try to return a useful filter | ||
| // even for yet unknown JDKs | ||
| int version = major - 45; | ||
| if ((version < 0)) { |
There was a problem hiding this comment.
Unnecessary parentheses around the condition. The expression if (version < 0) is clearer and follows standard Java style.
| if ((version < 0)) { | |
| if (version < 0) { |
|
|
||
|
|
There was a problem hiding this comment.
Unnecessary blank lines. Remove the extra blank line to maintain consistent formatting with the rest of the test file.
|
|
||
|
|
There was a problem hiding this comment.
Unnecessary blank lines. Remove the extra blank lines to maintain consistent formatting with the rest of the test file.
| assertEquals("(&(osgi.ee=JavaSE)(version=11))", result); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Unnecessary blank line. Remove the extra blank line to maintain consistent formatting with the rest of the test file.
Closes #6858
Experimental:
For a yet to bnd unknown JDK (e.g. JDK-10000) Instead of
we now create
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=9955))"
for a .class file with u2 major_version 10000 (9955 == 10000 - 45)