netbeans icon indicating copy to clipboard operation
netbeans copied to clipboard

Update FlatLaf from 2.4 to 2.5

Open DevCharly opened this issue 3 years ago • 17 comments

Update FlatLaf to v2.5.

Changes: https://github.com/JFormDesigner/FlatLaf/releases/tag/2.5

DevCharly avatar Sep 28 '22 10:09 DevCharly

Change looks sane to me, however I suggest to include this in this change set (if in doubt, feel free to squash it into your commit):

https://github.com/apache/netbeans/commit/66716b5506d91257f8ea136ffb11a669bac53f3a

The intention is to fix the issue described in #4619. @neilcsmith-net it would be nice if you could have a look.

matthiasblaesing avatar Sep 30 '22 19:09 matthiasblaesing

@matthiasblaesing Done. Thanks.

DevCharly avatar Sep 30 '22 20:09 DevCharly

@mbastian could you try whether the added implementation version works with your FlatLaf-SwingX module?

DevCharly avatar Sep 30 '22 20:09 DevCharly

@matthiasblaesing that change looks a good idea.

I wonder if we should be pre-extracting the native libs similarly to how JNA module does it here?

Aside @DevCharly - while looking at FlatLaf code for native loading noticed the comments on libjawt. Similar to #4441 and https://github.com/java-native-access/jna/pull/1422 Probably good to always preload.

neilcsmith-net avatar Oct 01 '22 11:10 neilcsmith-net

@matthiasblaesing that change looks a good idea.

Thanks for having a look.

I wonder if we should be pre-extracting the native libs similarly to how JNA module does it here?

In theory a good idea, in practice extracting libraries needs a way to force loading through System#loadLibrary with the basename only. In FlatLaf the loading always goes through extraction and loading target with full path. We'd need to modify this.

matthiasblaesing avatar Oct 01 '22 20:10 matthiasblaesing

In FlatLaf the loading always goes through extraction and loading target with full path.

Yes, I saw. We'd need to set this in the module installer.

Let's merge as is, but keep this in mind if we get any linking error reports.

@DevCharly an option in FlatLaf to prefer trying to load via System::loadLibrary without full path would be great in future!

neilcsmith-net avatar Oct 02 '22 11:10 neilcsmith-net

@neilcsmith-net thank you for the pointer - I looked one level to deep. What do you think about this:

https://github.com/apache/netbeans/commit/6781772b899fdfeaf855ed6b20d071b8bcf5fe60

@DevCharly the referenced commit should change the library loading, so that the library is loaded from the pre-extracted version in the modules directory. The issue with self extracting libraries is two fold: The first problem is cleanup - windows only lets you remove libraries after the users are closed, the second problem is on linux the temp directory could be mounted noexec, which also prevends library loading. As NetBeans needs to be executed, it can be expected, that the modules are located in an executable location.

matthiasblaesing avatar Oct 02 '22 15:10 matthiasblaesing

Thanks @matthiasblaesing Possibly should go directly in modules/lib rather than modules/lib/flatlaf though? The lib folder has a defined structure. Also, InstalledFileLocator can handle directories.

neilcsmith-net avatar Oct 02 '22 17:10 neilcsmith-net

@neilcsmith-net valid point updated to:

https://github.com/apache/netbeans/commit/be4dba61b9f6aa74089798095663fa3b917923d8

@DevCharly if you consider this, please use this commit, not the previous one, I replaced it based on Neils comments.

matthiasblaesing avatar Oct 02 '22 19:10 matthiasblaesing

@matthiasblaesing have added your commit (removed an unused import)

@DevCharly an option in FlatLaf to prefer trying to load via System::loadLibrary without full path would be great in future!

I wonder where Java/NetBeans searches for libraries if System::loadLibrary is used?

DevCharly avatar Oct 04 '22 20:10 DevCharly

I wonder where Java/NetBeans searches for libraries if System::loadLibrary is used?

In NetBeans, from modules/lib where the commit puts it. The classloader is used for locating native libraries, and the NetBeans classloaders look there.

neilcsmith-net avatar Oct 04 '22 20:10 neilcsmith-net

I wonder if we should be pre-extracting the native libs similarly to how JNA module does it here?

Is it possible that this is broken? When I start NB 15 (on Windows), JNA creates jna14122183419442360685.dll in C:\Users\abc\AppData\Local\Temp\jna--1361632413.

Looking at the JNA code, it seems that JNA uses System.loadLibrary() only if system property jna.nosys is set to false, which seems not the case. https://github.com/java-native-access/jna/blob/bf60e51eace6dffa18548019e2ba398ff84904ef/src/com/sun/jna/Native.java#L1003

DevCharly avatar Oct 05 '22 11:10 DevCharly

The implementation in above line (in previous post) seems not fit to the Javadoc: https://github.com/java-native-access/jna/blob/bf60e51eace6dffa18548019e2ba398ff84904ef/src/com/sun/jna/Native.java#L83

Also hint at following line is IMHO not useful because jna.nosys=true is the default: https://github.com/java-native-access/jna/blob/bf60e51eace6dffa18548019e2ba398ff84904ef/src/com/sun/jna/Native.java#L234

DevCharly avatar Oct 05 '22 11:10 DevCharly

Well, jna.nosys used to default to false. Changed in JNA 5.x I think (@matthiasblaesing ?). Possible this should have been updated in the installer here - https://github.com/apache/netbeans/blob/master/platform/libs.jna/src/org/netbeans/libs/jna/Installer.java#L29 Take to another issue?

neilcsmith-net avatar Oct 05 '22 11:10 neilcsmith-net

@DevCharly: You are right - it currently hits the "load from classpath" case @neilcsmith-net You are also right. The issue comes from the fix for https://github.com/java-native-access/jna/issues/384. I think the change was sane, but indeed NetBeans needs to be updated to handle this.

matthiasblaesing avatar Oct 05 '22 17:10 matthiasblaesing

The jna issue should be fixed by #4736

matthiasblaesing avatar Oct 05 '22 17:10 matthiasblaesing

@DevCharly sorry, looks like our PRs conflicted in changes to Installer. Are you able to update and resolve?

neilcsmith-net avatar Oct 11 '22 08:10 neilcsmith-net

Opened #4803 with conflicts resolved instead.

neilcsmith-net avatar Oct 17 '22 11:10 neilcsmith-net