Try all frameworks paths regardless of file exist check. #1216

dkocher · 2020-06-28T20:40:18Z

New in macOS Big Sur 11 beta, the system ships with a built-in dynamic linker cache of all system-provided libraries. As part of this change, copies of dynamic libraries are no longer present on the filesystem. Code that attempts to check for dynamic library presence by looking for a file at a path or enumerating a directory will fail. Instead, check for library presence by attempting to dlopen() the path, which will correctly check for the library in the cache. (62986286)

Fix #1215.

Signed-off-by: David Kocher [email protected]

matthiasblaesing · 2020-06-29T19:26:06Z

Could you please have a look at the result of the CI build? https://travis-ci.org/github/java-native-access/jna/jobs/702961796

The change introduces unittest failures.

msm · 2020-06-29T22:17:30Z

Could you please have a look at the result of the CI build? https://travis-ci.org/github/java-native-access/jna/jobs/702961796

The change introduces unittest failures.

(not the OP but took a look at their commit)

It looks like the failing test is essentially using comparing new File(..).exists() logic with NativeLibrary.matchFramework(..)'s behavior:

public void testMatchFramework() {
        if (!Platform.isMac()) {
            return;
        }
        final String[][] MAPPINGS = {
            // Depending on the system, /Library/Frameworks may or may not
            // have anything in it.
            { "QtCore", expected("/Library/Frameworks/QtCore.framework/QtCore") },
            { "Adobe AIR", expected("/Library/Frameworks/Adobe AIR.framework/Adobe AIR") },

            { "QuickTime", expected("/System/Library/Frameworks/QuickTime.framework/QuickTime") },
            { "QuickTime.framework/Versions/Current/QuickTime", expected("/System/Library/Frameworks/QuickTime.framework/Versions/Current/QuickTime") },
        };
        for (int i=0;i < MAPPINGS.length;i++) {
            assertEquals("Wrong framework mapping", MAPPINGS[i][1], NativeLibrary.matchFramework(MAPPINGS[i][0]));
        }
    }

Previously, this made sense since the matchFramework(..) method was designed to look for potential library paths that exist.

By definition, this won't work anymore with this new change's strategy - per the Apple release note, system-provided libraries won't necessarily exist anymore, yet they will be magically available if you dlopen() them.

In other words, unless there is a reason not to try to dlopen(..) paths that may not exist, I think this specific unit test should be removed as the behavior it is trying to test is no longer something we want.

dbwiddis · 2020-06-30T06:05:50Z

It seems the logic here tries to solve all problems at once, but probably handles more cases than it should.

I generally avoid premature optimization, but we're incurring multiple filesystem accesses here without a good reason. If the file is found (which should only happen on 10.x), why do we not just return it there instead of adding to the list and continuing to (unnecessarily) add more to the path?
We assume if the file is not found that we must be on macOS 11 and load all the paths. But that's not appropriate if we're on 10.x. We can easily check System.getProperty("os.version") without incurring an (attempted) filesystem read, and react appropriately
- If we know we're on macOS 10.x why do anything other than what we currently do (find the file and return it)?
- If we know we're on macOS 11.x why try to find the file, instead of going straight to the new behavior?

Previously, this made sense

By definition, this won't work anymore

It still does make sense and works for 10.x. Switching on os.version can also be applied to the testcase. I'd favor keeping the existing test with a conditional if we're on 10.x, and (if possible/feasible) adding a new appropriate test if we're on 11.x.

msm · 2020-06-30T16:18:51Z

If we know we're on macOS 10.x why do anything other than what we currently do (find the file and return it)?

If we know we're on macOS 11.x why try to find the file, instead of going straight to the new behavior?

So, I may be missing something obvious here, but one question: this new behavior on macOS 11.x is for "system-provided" libraries. Previously, we poked around and attempted to find them directly on the file system. Now, they magically come back if you ask for them, presumably by just their name, not with a fake path?

What if, when System.getProperty("os.version") indicates that we are on macOS 11.x, we simply attempt to dlopen() the library name at the end of the chain if we didn't find the library on the filesystem?

E.g., instead of changing the logic to try all framework paths regardless of file presence, instead just do a "blind" dlopen(..) as a latch-ditch attempt if we haven't found anything more specific on the filesystem (with the "old" logic) and are running on macOS 11.x?

dbwiddis

Some more comments on the code, but I think we still have an open discussion on the overall logic.

dbwiddis · 2020-06-30T15:23:24Z

test/com/sun/jna/NativeLibraryTest.java

-        };
-        for (int i=0;i < MAPPINGS.length;i++) {
-            assertEquals("Wrong framework mapping", MAPPINGS[i][1], NativeLibrary.matchFramework(MAPPINGS[i][0]));
+        if (!System.getProperty("os.version").startsWith("10")) {


You can probably simplify by including this in the previous conditional. Also for the comment rather than "10.11" should say either "macOS 11" or "11.x".

dbwiddis · 2020-06-30T15:25:41Z

src/com/sun/jna/NativeLibrary.java

-                libraryPath = matchFramework(libraryName);
-                if (libraryPath != null) {
+                for(String frameworkName : matchFramework(libraryName)) {
+                    libraryName = frameworkName;


I'm missing why this assignment is needed.

I'm missing why this assignment is needed.

In fact, this causes a problem for the later attempt to find a library in an embedded jar. By assigning the various attempts to libraryPath (e.g., mutating that variable) we end up trying to find an embedded jar in ${os-prefix}/libraryPath with whatever the last "guess" value was for libraryPath.

Removing that assignment and instead passing frameworkName to the Native.open(frameworkName, openFlags) call fixes this issue by not mangling libraryPath for the later attempt to open from the JAR

But that change was backed out from the final commit. Here's the actual changes.

dbwiddis · 2020-06-30T15:29:21Z

src/com/sun/jna/NativeLibrary.java

            }
        }
-        return null;
+        return paths.toArray(new String[paths.size()]);


Minor point: the [0] version of toArray is slightly more efficient, and we went through and changed them all back in #1060 so best to be consistent here.

dbwiddis · 2020-06-30T15:30:52Z

test/com/sun/jna/NativeLibraryTest.java

        }
+        // Depending on the system, /Library/Frameworks may or may not have anything in it.
+        assertEquals("Wrong framework mapping", "/Library/Frameworks/QtCore.framework/QtCore",
+                NativeLibrary.matchFramework("QtCore")[2]);


I'm not understanding why the array index 2 here and the next two entries, but 0 for QuickTime?

msm · 2020-06-30T17:05:08Z

test/com/sun/jna/NativeLibraryTest.java

-        };
-        for (int i=0;i < MAPPINGS.length;i++) {
-            assertEquals("Wrong framework mapping", MAPPINGS[i][1], NativeLibrary.matchFramework(MAPPINGS[i][0]));
+        if (!System.getProperty("os.version").startsWith("10")) {


Note: System.getProperty("os.version") currently returns 10.16 for developer beta 1. My understanding is this will eventually become 11.00 (or possibly 11.0?) in a subsequent build, and may already be reporting that on the Apple DTK ARM build. But for the time being this check on just startsWith("10") won't work on DP1

~~System.getProperty("os.version").compareTo("10.15") > 0 should work.~~

(EDIT: or not, given I'm currently on 10.15.5. Comparing >= 10.16 should work!)

System.getProperty("os.version").compareTo("10.16") >= 0 should work.

dkocher · 2020-06-30T19:12:39Z

If we know we're on macOS 10.x why do anything other than what we currently do (find the file and return it)?

If we know we're on macOS 11.x why try to find the file, instead of going straight to the new behavior?

So, I may be missing something obvious here, but one question: this new behavior on macOS 11.x is for "system-provided" libraries. Previously, we poked around and attempted to find them directly on the file system. Now, they magically come back if you ask for them, presumably by just their name, not with a fake path?

What if, when System.getProperty("os.version") indicates that we are on macOS 11.x, we simply attempt to dlopen() the library name at the end of the chain if we didn't find the library on the filesystem?

E.g., instead of changing the logic to try all framework paths regardless of file presence, instead just do a "blind" dlopen(..) as a latch-ditch attempt if we haven't found anything more specific on the filesystem (with the "old" logic) and are running on macOS 11.x?

Probably makes sense to simplify this way and only do the various path concatenation when running on earlier versions. Will do the System.getProperty("os.version").compareTo("10.16") >= 0 which should be future proof.

dkocher · 2020-06-30T19:16:36Z

Let me know if I should sqash the commits.

dbwiddis · 2020-06-30T19:31:58Z

Let me know if I should sqash the commits.

Generally before merging, yes. For complex changes, I don't mind seeing individual commits during review so we can see what has changed in each commit.

You can squash when you add a changelog entry. :-)

dbwiddis · 2020-06-30T23:41:57Z

This looks good to me. I'll leave it for anyone else to comment.

dbwiddis · 2020-07-01T16:36:56Z

Merged in 65085c1

msm · 2020-07-02T04:05:08Z

Welp, I may have been trying to be too clever too quickly with my previous suggestion to do a "blind" dlopen(..) as a latch-ditch attempt on macOS 11.

@dkocher were you able to successfully access a native framework library, e.g. Carbon, with this version of code?

dlopen() fails for me if I pass it simply Carbon or libCarbon by itself, on macOS 11 DP1. That works fine from previous versions of macOS, and that is how other popular libraries consume JNA currently.

When I build JNA using your first attempt that went through the previous logic of coming up with possible paths to try to open, but didn't do the file-exists-check, that did work, and allow me to invoke Carbon APIs as expected.

Is my experience different from yours? Are you able to load a system framework with the currently-merged code in JNA?

dkocher · 2020-07-02T07:23:44Z

@dkocher were you able to successfully access a native framework library, e.g. Carbon, with this version of code?

I mean to have this verified with loading Foundation.framework on macOS 11 DP1. I will double check I have indeed tested the latest revision.

dkocher · 2020-07-02T07:45:12Z

@dkocher were you able to successfully access a native framework library, e.g. Carbon, with this version of code?

dlopen() fails for me if I pass it simply Carbon or libCarbon by itself, on macOS 11 DP1. That works fine from previous versions of macOS, and that is how other popular libraries consume JNA currently.

I forgot to install the latest build into my local Maven repository and thus tested with a previous build :( I can confirm loading with Foundation or Foundation.framework fails (neither on macOS 10.15 or 11DP1). As I have quoted initially one has to pass the full path to the framework like /System/Library/Frameworks/Foundation.framework/Foundation.

Thus I propose to revert to a variant of my initial patch.

… coverage. > Check for library presence by attempting to dlopen() the path, which will correctly check for the library in the cache. Signed-off-by: David Kocher <[email protected]>

dbwiddis · 2020-07-02T17:44:38Z

I'll wait until @msm tests this latest version before merging again. Thanks for updating.

msm · 2020-07-03T15:15:54Z

I have tested the updated fix for loading frameworks on macOS 11. I am happy to report it is working now for both system frameworks and loading embedded libraries in jars.

@dkocher Thanks for updating!

@dbwiddis Would it be possible to work towards releasing a 5.6 or similar version in the near future? There are a few other projects that have issues reported against them that this would fix:

gmethvin/directory-watcher#52
https://youtrack.jetbrains.com/issue/IDEA-244962?p=JBR-2535

dbwiddis · 2020-07-03T18:15:38Z

OK, I'll get this merged (again!). Regarding a 5.6.0 release, I suggest starting a thread on the mailing list.

Changes to the latest beta version of macOS broke the mechanism that we're using to observe filesystem change events. It's more than just filesystem observation that has broken: any attempt to run a hot-reloading workflow crashes the Figwheel process with an uncaught exception at startup. (Note that build-once still works, since it doesn't try to access filesystem events). The underlying hawk library does have an implementation of a less-efficient polling watcher. Figwheel can use this to provide hot-reloading even when native events aren't available. This commit wraps the `hawk.core/watch!` function to catch any exception that's thrown, and re-try with the same options *except* that it explicitly starts a polling watcher. See also - gjoseph/BarbaryWatchService#13 - java-native-access/jna#1216 Part of bhauman#253.

dbwiddis · 2020-07-07T21:02:15Z

@dkocher , @msm Would you be able to test the 5.6.0 snapshot on the macOS 11 beta? See the mailing list thread for links.

Mac OS 11 (aka. "Big Sur") made some changes to the way that the Carbon library is located, causing JNA calls to fail. This has been fixed in JNA 5.6.0; see java-native-access/jna#1216 Since Barbary is implemented on JNA, this is causing downstream projects that rely on file watching to crash on JVM start when used on Mac OS 11. Examples: - bhauman/figwheel-main#253 - thheller/shadow-cljs#767

dbwiddis reviewed Jun 30, 2020

View reviewed changes

msm reviewed Jun 30, 2020

View reviewed changes

dkocher force-pushed the issue-1215 branch from bd64333 to 3452531 Compare June 30, 2020 21:04

dbwiddis force-pushed the master branch from 8a44e4c to 22dc037 Compare July 1, 2020 16:16

dbwiddis closed this Jul 1, 2020

msm mentioned this pull request Jul 1, 2020

LibCarbon failure on MacOS Big Sur gmethvin/directory-watcher#52

Closed

dbwiddis reopened this Jul 2, 2020

dkocher force-pushed the issue-1215 branch from c18ece6 to 8564503 Compare July 2, 2020 11:44

Revert to previous fix opening frameworks with full path. Adjust test…

e6c6b37

… coverage. > Check for library presence by attempting to dlopen() the path, which will correctly check for the library in the cache. Signed-off-by: David Kocher <[email protected]>

dkocher force-pushed the issue-1215 branch from a865143 to e6c6b37 Compare July 2, 2020 14:11

This was referenced Jul 3, 2020

libCarbon.dylib fails to load on MacOS Big Sur gjoseph/BarbaryWatchService#13

Closed

File watching fails on MacOS Big Sur beta bhauman/figwheel-main#253

Closed

dbwiddis merged commit e6c6b37 into java-native-access:master Jul 3, 2020

rgm mentioned this pull request Jul 7, 2020

Add a polling fallback to filesystem watcher bhauman/figwheel-main#254

Closed

17hao mentioned this pull request Jul 15, 2020

Update jna version from 5.5 to 5.6 gmethvin/directory-watcher#53

Merged

rgm mentioned this pull request Aug 4, 2020

Update jna dependency to restore native file watching on Macs gjoseph/BarbaryWatchService#14

Closed

nguyenq mentioned this pull request Aug 12, 2020

Homebrew installed Tesseract not detected by Tess4J nguyenq/tess4j#194

Open

bpossolo mentioned this pull request Feb 27, 2021

sbt-sassify broken on Apple Silicon M1 + Big Sur irundaia/sbt-sassify#48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try all frameworks paths regardless of file exist check. #1216

Try all frameworks paths regardless of file exist check. #1216

dkocher commented Jun 28, 2020

matthiasblaesing commented Jun 29, 2020

msm commented Jun 29, 2020

dbwiddis commented Jun 30, 2020

msm commented Jun 30, 2020

dbwiddis left a comment

dbwiddis Jun 30, 2020

dbwiddis Jun 30, 2020

msm Jul 2, 2020

dbwiddis Jul 2, 2020

dbwiddis Jun 30, 2020

dbwiddis Jun 30, 2020

msm Jun 30, 2020

dbwiddis Jun 30, 2020 •

edited

Loading

dkocher commented Jun 30, 2020

dkocher commented Jun 30, 2020

dbwiddis commented Jun 30, 2020

dbwiddis commented Jun 30, 2020

dbwiddis commented Jul 1, 2020

msm commented Jul 2, 2020

dkocher commented Jul 2, 2020

dkocher commented Jul 2, 2020

dbwiddis commented Jul 2, 2020

msm commented Jul 3, 2020

dbwiddis commented Jul 3, 2020

dbwiddis commented Jul 7, 2020

Try all frameworks paths regardless of file exist check. #1216

Try all frameworks paths regardless of file exist check. #1216

Conversation

dkocher commented Jun 28, 2020

matthiasblaesing commented Jun 29, 2020

msm commented Jun 29, 2020

dbwiddis commented Jun 30, 2020

msm commented Jun 30, 2020

dbwiddis left a comment

Choose a reason for hiding this comment

dbwiddis Jun 30, 2020

Choose a reason for hiding this comment

dbwiddis Jun 30, 2020

Choose a reason for hiding this comment

msm Jul 2, 2020

Choose a reason for hiding this comment

dbwiddis Jul 2, 2020

Choose a reason for hiding this comment

dbwiddis Jun 30, 2020

Choose a reason for hiding this comment

dbwiddis Jun 30, 2020

Choose a reason for hiding this comment

msm Jun 30, 2020

Choose a reason for hiding this comment

dbwiddis Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

dkocher commented Jun 30, 2020

dkocher commented Jun 30, 2020

dbwiddis commented Jun 30, 2020

dbwiddis commented Jun 30, 2020

dbwiddis commented Jul 1, 2020

msm commented Jul 2, 2020

dkocher commented Jul 2, 2020

dkocher commented Jul 2, 2020

dbwiddis commented Jul 2, 2020

msm commented Jul 3, 2020

dbwiddis commented Jul 3, 2020

dbwiddis commented Jul 7, 2020

dbwiddis Jun 30, 2020 •

edited

Loading