Skip to content

[NETBEANS-5499] Upgrade Felix library, works on JDK16.#2873

Merged
geertjanw merged 3 commits intoapache:masterfrom
sdedic:sdedic/upgrade-felix-jdk16
Apr 16, 2021
Merged

[NETBEANS-5499] Upgrade Felix library, works on JDK16.#2873
geertjanw merged 3 commits intoapache:masterfrom
sdedic:sdedic/upgrade-felix-jdk16

Conversation

@sdedic
Copy link
Member

@sdedic sdedic commented Apr 8, 2021

I have tried Felix 7.0.0 with a minimal application and it seems that URL handling is fixed.

@sdedic sdedic added kind:bug Bug report or fix Upgrade Library Library (Dependency) Upgrade labels Apr 8, 2021
@sdedic sdedic added this to the 12.4 milestone Apr 8, 2021
@sdedic sdedic force-pushed the sdedic/upgrade-felix-jdk16 branch from 567adae to 34577c3 Compare April 8, 2021 10:09
@sdedic sdedic force-pushed the sdedic/upgrade-felix-jdk16 branch from 34577c3 to 2bac8ef Compare April 8, 2021 10:27
@sdedic
Copy link
Member Author

sdedic commented Apr 8, 2021

Recommitted with the necessary project.{xml,properties} changes. Sorry for the inconvenience.

@sdedic sdedic self-assigned this Apr 8, 2021
@JaroslavTulach
Copy link

Test org.netbeans.core.netigso.EnabledAutoloadTest FAILED

Copy link

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Once the tests pass, feel free to integrate.

@sdedic sdedic force-pushed the sdedic/upgrade-felix-jdk16 branch from b431dcb to 5aa9dff Compare April 16, 2021 07:47
@sdedic
Copy link
Member Author

sdedic commented Apr 16, 2021

Had to upgrade OSGi as well, Felix 7.0 seems to require OSGI 8 core. osgi-cmp remains at 7.0 (Compendium 8.0 spec is still draft) while osgi-core 8.0 was released in December 2020.

Please re-review.

Copy link

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Would having a test failing on JDK16 before and succeeding with this change be beneficial?

@sdedic
Copy link
Member Author

sdedic commented Apr 16, 2021

Would having a test failing on JDK16 before and succeeding with this change be beneficial?

Can you suggest how to proceed with such test implementation ? i.e. two variants of the Felix (OSGi ?) library wrapper modules - how ? Or some simpler way ?

@JaroslavTulach
Copy link

Can you suggest how to proceed with such test implementation ? ... Or some simpler way ?

The problems only appears on JDK16, right? We should have JDK16 gate running commit validation and failing with some older commit like 5a9c949 - then apply this PR and the gate starts passing. Simple.

@geertjanw
Copy link
Member

Is this ready to merge, for 12.4 inclusion, please do so if so @JaroslavTulach @sdedic.

@sdedic
Copy link
Member Author

sdedic commented Apr 16, 2021

@jtulach I have modified a commit-validation test to parse an URL - which (on my machine) fails on JDK16 without the felix change. But I failed to enable commit-validation testsuite on JDK16. At least locally, I need a ton of --add-opens (copied from etc/netbeans.conf) and even then a consistency test fails:

Some files do not provide valid menu elements[ File MultiFileObject@115e5315[Toolbars/Memory/org-netbeans-modules-timers-OpenTimeComponentAction.shadow] does not provide correct instance: java.lang.Object@739d42c5 url: nbfs://nbhost/SystemFileSystem/Toolbars/Memory/org-netbeans-modules-timers-OpenTimeComponentAction.shadow]

@sdedic sdedic force-pushed the sdedic/upgrade-felix-jdk16 branch from d0f4429 to 3385dd4 Compare April 16, 2021 17:41
@geertjanw geertjanw merged commit b8ab5e2 into apache:master Apr 16, 2021
@sdedic
Copy link
Member Author

sdedic commented Apr 16, 2021

@geertjanw there was a real travis failure before the last push (that's why I recommitted the changes). Please let the builds for PRs to finish / settle ... I hope the master won't be broken :)

@geertjanw
Copy link
Member

OK. Delivery branch exists. Please say when you're happy and will then continue the process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:bug Bug report or fix Upgrade Library Library (Dependency) Upgrade

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants