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

GIMP plugin: Link against the shared library. #573

Merged
merged 1 commit into from Sep 10, 2021
Merged

Conversation

deymo
Copy link
Contributor

@deymo deymo commented Sep 10, 2021

Now that the plugin only uses the external libjxl API we can link
against the shared library. This reduces the binary size from 3.9 MB to
49 KB (both after stripping) which is important from a distribution
point of view.

Now that the plugin only uses the external libjxl API we can link
against the shared library. This reduces the binary size from 3.9 MB to
49 KB (both after stripping) which is important from a distribution
point of view.
@deymo deymo requested a review from veluca93 September 10, 2021 15:04
@deymo
Copy link
Contributor Author

deymo commented Sep 10, 2021

@xiota FYI.

@deymo
Copy link
Contributor Author

deymo commented Sep 10, 2021

I'd propose this to merge for v0.6, WDYT @veluca93 ?

@jonsneyers
Copy link
Member

Can we get this and also #525 merged for v0.6?

@veluca93
Copy link
Member

I'd propose this to merge for v0.6, WDYT @veluca93 ?

yup

@xiota
Copy link
Contributor

xiota commented Sep 10, 2021

I think it would be beneficial to have an additional static build, like how the oneshot examples do. On platforms, like windows, installing the dlls is a pain, so it would be good for the plugin to just work.

@jonsneyers jonsneyers added the building/portability Platform-specific issues, build issues label Sep 10, 2021
@jonsneyers
Copy link
Member

A static build for windows, dynamic linking for linux distros is probably the most convenient indeed. Can we do it like that? (make it platform-dependent what is done)

@deymo
Copy link
Contributor Author

deymo commented Sep 10, 2021

If either TARGET_SUPPORTS_SHARED_LIBS is false (unlikely, except in some embedded systems) or you pass -DJPEGXL_STATIC=ON to CMake then jxl and jxl_threads are aliases of the respective -static versions. If you want this plugin to be static, probably the same reasoning applies to cjxl/djxl. There's maybe a third option which would be to only link jxl targets statically. Statically linking is in general not great so I wouldn't make it the default. Copying/installing the .dll next to the .exe is probably better than bundling them.

@xiota
Copy link
Contributor

xiota commented Sep 10, 2021

Copying/installing the .dll next to the .exe is probably better than bundling them.

That's part of the pain of dlls in windows. Just copying the dll next to the exe doesn't work in modern windows, at least my attempts failed. libjxl dlls also have lots of dependencies that I couldn't figure out how to resolve on windows. The oneshot examples build both dynamic and static versions. It would be nice for the plugin to do the same. Also, since the API is changing, it would allow users to continue to use the old binary until the plugin is updated.

(I'm in favor of building the plugin dynamically, but think also having the static version is beneficial.)

@deymo
Copy link
Contributor Author

deymo commented Sep 10, 2021

ok, I'll merge this.

libjxl.dll depends on brotli, and probably nothing else since we link the shared lib against the static skcms.

Does -DJPEGXL_STATIC=ON solve the issue? I haven't tested it on native windows builds.

@deymo deymo merged commit 3246b05 into libjxl:main Sep 10, 2021
@deymo deymo deleted the gimp_shared branch September 10, 2021 16:33
@deymo deymo added the merge-0.6 PR label to flag PRs that need to be merged to v0.6.x label Sep 10, 2021
@xiota
Copy link
Contributor

xiota commented Sep 10, 2021

libjxl.dll depends on brotli, and probably nothing else since we link the shared lib against the static skcms.

Does -DJPEGXL_STATIC=ON solve the issue? I haven't tested it on native windows builds.

Last time I tried building on Windows was over a week ago, around when I wrote the msys and crossroad guides. There were libpng and half a dozen other dll dependencies. Trying to build with -DJPEGXL_STATIC=ON failed. I'll try again.

@deymo
Copy link
Contributor Author

deymo commented Sep 10, 2021

Last time I tried building on Windows was over a week ago, around when I wrote the msys and crossroad guides. There were libpng and half a dozen other dll dependencies. Trying to build with -DJPEGXL_STATIC=ON failed. I'll try again.

cjxl.exe and djxl.exe will depend on all those; but not jxl.dll. It is possible that -DJPEGXL_STATIC=ON fails on native windows because of the different ways stdc++ and threads dependencies interact in different platforms. What we can easily do is to add a -DJPEGXL_STATIC_LIBS=ON to make libjxl alias libjxl-static or just tie that to the more standard -DBUILD_SHARED_LIBS=OFF cmake flag.

@xiota
Copy link
Contributor

xiota commented Sep 10, 2021

cjxl.exe and djxl.exe will depend on all those; but not jxl.dll. It is possible that -DJPEGXL_STATIC=ON fails on native windows because of the different ways stdc++ and threads dependencies interact in different platforms. What we can easily do is to add a -DJPEGXL_STATIC_LIBS=ON to make libjxl alias libjxl-static or just tie that to the more standard -DBUILD_SHARED_LIBS=OFF cmake flag.

Is that how the static oneshot binaries are built?

@deymo
Copy link
Contributor Author

deymo commented Sep 10, 2021

The oneshot examples are not built statically as part of the libjxl project. There's an examples/CMakeLists.txt example project that show how would you include libjxl statically; but we don't include that file in libjxl project itself. That file is meant to demonstrate how to write a cmake-based project with pkg-config that uses libjxl both statically and with the shared library. We build that in one of the CI workflows to make sure that it works, but not as part of the libjxl cmake run. What we include is examples/examples.cmake which uses the internally-defined "jxl" and "jxl_threads" targets. If you pass -DBUILD_SHARED_LIBS=OFF to libjxl, those would be built using the statilc libs (after #575). The gimp plugin and gdk plugins are currently built inside the libjxl project. At this point they could be split out as different projects that just use the external api and include the libjxl, but for development it is easier to have them build together with the library.

deymo added a commit to deymo/libjxl that referenced this pull request Oct 4, 2021
Now that the plugin only uses the external libjxl API we can link
against the shared library. This reduces the binary size from 3.9 MB to
49 KB (both after stripping) which is important from a distribution
point of view.
(cherry picked from commit 3246b05)
(cherry picked from PR libjxl#573)
deymo added a commit that referenced this pull request Oct 4, 2021
Now that the plugin only uses the external libjxl API we can link
against the shared library. This reduces the binary size from 3.9 MB to
49 KB (both after stripping) which is important from a distribution
point of view.
(cherry picked from commit 3246b05)
(cherry picked from PR #573)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building/portability Platform-specific issues, build issues merge-0.6 PR label to flag PRs that need to be merged to v0.6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants