jasper-software / jasper

Official Repository for the JasPer Image Coding Toolkit
http://www.ece.uvic.ca/~mdadams/jasper
Other
217 stars 103 forks source link

[hotfix] Do not enforce rpath when building shared library #347

Closed uilianries closed 5 months ago

uilianries commented 1 year ago

Greetings!

Conan has been packaged Jasper for a time and one thing which we always need to do is disabling the rpath configuration hardcoded in the CMakeLists.txt. As side-effect, once the library is packaged, its rpath will point to where it was created, in that case, the CI environment. It's just example, but it would occur for anyone who build Jasper locally and tries to distribute.

Those kind of configuration can be manually configured by a CMake toolchain file apart or by command line definitions.

/cc @SpaceIm

jubalh commented 1 year ago

@MaxKellermann can I ask your opinion of this?

mdadams commented 1 year ago

@jubalh I have had problems in the past with an incompatible version of the jasper shared library being linked against when doing jasper development due to the rpath settings in the build tree not forcing the development version of the library to get picked up (instead of one already installed on the system). When this happens, it can often waste a fair amount of time to isolate the problem, since the code would fail in very subtle ways and checking the rpath is not always the first thing that one would think to check. This would be my main concern with changes to rpath handling. I would like to ensure that this type of situation cannot happen (even if the shared library version has not yet been bumped, since this is usually not done until just before a release). I would have the check the CMake rpath documentation to determine if the change being proposed in the PR would impact this particular situation of concern. Without checking the documentation, I am unsure whether my above concern is relevant or not. I just thought I would mention this in case it might be.

mdadams commented 5 months ago

@uilianries In commit faeed11274db2c864520a3246a51bf014ae26fe3, I have just added a JAS_PACKAGING option to the CMake build for JasPer. When enabled, this option prevents the CMakeLists.txt file from setting any of the rpath-related variables in CMake. Does this meet your needs? You can now set JAS_PACKAGING to true and then set whatever CMake rpath settings that you want on the cmake command line. This allows you and other packagers to avoid the pain of having to patch the CMakeLists.txt file, but allows more typical users (i.e., people who are not packaging JasPer) to continue to have the old behavior (which is more desirable for them).

mdadams commented 5 months ago

@uilianries The new release of JasPer (4.2.0) includes the JAS_PACKAGING CMake option that I mentioned above. I believe that this should address the issue that you have raised, so I am closing this PR. If this is not a satisfactory solution, please let me know.

uilianries commented 5 months ago

Thank you!!!! 🎉