HumbleUI / Skija

Java bindings for Skia
Apache License 2.0
498 stars 34 forks source link

Improve native library loading #34

Closed Glavo closed 1 year ago

Glavo commented 1 year ago

Incompatible change:

This PR introduces large changes and may require more testing before being merged.

Glavo commented 1 year ago

Purpose of this PR:

tonsky commented 1 year ago

Thanks for the PR! Adding _arch everywhere is a good call, I was meaning to do it myself eventually.

Lots of good stuff, but hard to accept all at once. It worked for me on Mac M1 and on Ubuntu x64. My windows toolchain is currently broken (not just on this PR, in general), so I can’t check yet

Some notes/questions I have:

And I would really prefer this to be split into smaller bite-size PRs

Glavo commented 1 year ago

@tonsky

skija.version is being put into platform jars (as it should) but loaded using classloader of shared jar. IIRC it doesn’t worked for me with Java 9 modules, that’s why I had to create a class in each platform

It's not true. I tested these situations. In these cases, ClassLoader performs well in locating resources:

When jlink is not used, there is no difference between using ClassLoader and using LibraryFinder to locate resources.

Only in the case of using jlink, there may be problems in some edge cases. However, I have examined this issue again these days, it is not difficult to solve. This problem can be solved by using ModuleFinder to find modules and locate resources. I just need a little time to achieve it.

one of the goals of having arch classifier in e.g. libskija_x64.dylib was to make sure I can include all possible skija jars at the same time and still get the right one loaded. I think by removing _x64/_arm64 you broke that? Now if I add both jars to class path there’ll be two identical resources which might be tricky to distinguish

Native libraries of different architectures are already in different packages, and they will not conflict in any case. I don't understand why it's even doing this.

I see you use both jmod command and ZipFile to produce .jmod. Shouldn’t one or the other be enough? Also, currently it seems to use no compression, resulting in 21 Mb file instead of 9 Mb for jar

The jmod command is difficult to configure. I ran into trouble when using the jmod command, which made it is difficult to place all resources where they should be by directly calling the jmod command

If I want to generate the correct jmod file only through the jmod command, I must create a temporary directory, copy the resource files to the corresponding layout location, and then call the jmod command. Compared with this, I think ZipFile is a cleaner solution.

The reason why I don't use ZipFile directly to create the jmod file is that there are magic numbers (0x4A 0x4D 0x01 0x00) at the beginning of the jmod file. It seems that there is no simple way to use ZipFile to write these magic numbers.

The lack of compression might be an oversight on my part, I'll check that out later.

Shouldn’t we publish jmod somewhere? Right now it’s built, but not published, so kind of misses the point, right?

I think the jmod file should be placed in GitHub Release? Or should we push it to Maven Central?

There seems to be no best practice for publishing jmod files, so you need to make a choice.

Will everything here continue to work with Java 8?

Of course, I certainly don't want to break the work I just finished.

And I would really prefer this to be split into smaller bite-size PRs

I agree that this is a big change. The reason why I make so many changes in a PR is that I think these changes are actually closely related. A part of them may be split into separate PRs, but the rest will still be no small modifications.

Do you think it must be split?

tonsky commented 1 year ago

Native libraries of different architectures are already in different packages, and they will not conflict in any case. I don't understand why it's even doing this.

Oh I see, the name is the same but base path is different. That works! My mistake

If I want to generate the correct jmod file only through the jmod command, I must create a temporary directory, copy the resource files to the corresponding layout location, and then call the jmod command.

Let’s do this? I just don’t like that two approaches are mixed

I think the jmod file should be placed in GitHub Release?

Let’s do that, yes

Do you think it must be split?

Not this one, but future ones please?

Glavo commented 1 year ago

@tonsky

Now I have made some updates:

Not this one, but future ones please?

I'm sorry that this PR is so large.

The original intention of this PR is to optimize the way to load the local library.

As part of this work, I went to adding _arch everywhere, but when I started adding, I found that there were more changes needed than I thought.

If I implement the functions in this PR from scratch, I will first add arch anywhere, and other functions can be split into separate PRs. But I don't want to do these things again, because it will take me a lot of time to test again.

I apologize for the trouble this has caused you.

tonsky commented 1 year ago

Ok, merged, tagged as 0.109.1, let’s wait until it appears in Maven Central and see if it works

tonsky commented 1 year ago

Looks like I fucked up GH Release artifacts upload, should be fixed in next release