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

Update FlatLaf from 2.4 to 2.5 #4696

Closed
wants to merge 3 commits into from
Closed

Conversation

DevCharly
Copy link
Member

@DevCharly DevCharly added Upgrade Library Library (Dependency) Upgrade Look and Feel labels Sep 28, 2022
@DevCharly DevCharly added this to the NB16 milestone Sep 28, 2022
…rsion

This allows modules that need access to the internal packages and
accept, that they will be version locked to declare an implementation
dependency on this module.
@matthiasblaesing
Copy link
Contributor

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):

66716b5

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

@DevCharly
Copy link
Member Author

@matthiasblaesing Done. Thanks.

@DevCharly
Copy link
Member Author

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

@neilcsmith-net
Copy link
Member

@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 java-native-access/jna#1422 Probably good to always preload.

@matthiasblaesing
Copy link
Contributor

@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.

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Oct 2, 2022

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!

@matthiasblaesing
Copy link
Contributor

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

6781772

@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.

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Oct 2, 2022

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.

@matthiasblaesing
Copy link
Contributor

@neilcsmith-net valid point updated to:

be4dba6

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

@DevCharly
Copy link
Member Author

@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?

@neilcsmith-net
Copy link
Member

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.

@DevCharly
Copy link
Member Author

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
Copy link
Member Author

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

@neilcsmith-net
Copy link
Member

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?

@matthiasblaesing
Copy link
Contributor

@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 java-native-access/jna#384. I think the change was sane, but indeed NetBeans needs to be updated to handle this.

@matthiasblaesing
Copy link
Contributor

The jna issue should be fixed by #4736

@neilcsmith-net
Copy link
Member

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

@neilcsmith-net
Copy link
Member

Opened #4803 with conflicts resolved instead.

@neilcsmith-net neilcsmith-net removed this from the NB16 milestone Oct 17, 2022
@DevCharly DevCharly deleted the flatlaf-2.5 branch January 23, 2023 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Look and Feel Upgrade Library Library (Dependency) Upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants