build: fix openjph target use #4894
Conversation
|
@kmilos Does this look right to you? |
lgritz
added
the
build / testing / port / CI
|
Anybody care to review this? It's the last piece I need to release a 3.1 beta2. |
src/cmake/modules/FindOpenJPEG.cmake
Outdated
| set_target_properties(openjp2 PROPERTIES | ||
| INTERFACE_INCLUDE_DIRECTORIES "${OPENJPEG_INCLUDES}") | ||
| set_property(TARGET openjp2 APPEND PROPERTY | ||
| IMPORTED_LOCATION "${OPENJPEG_LIBRARIES}") |
There was a problem hiding this comment.
I am not 100% sure this is sufficient - e.g. my OpenJPEGTargets-release.cmake config on Windows also has an IMPORTED_IMPLIB property for the stub to go w/ IMPORTED_LOCATION DLL...
I think it might be safer to do the minimal change just for the openjph target first (maybe something like I suggested), and only then refactor this later. This FindOpenJPEG module feels like overkill as OpenJPEG (at least v2) also ships CMake config files for a long time, maybe it should be checked first before setting stuff up manually...
There was a problem hiding this comment.
I should check if it's still the case, but last time I looked at using the OpenJPEG's own exported cmake configs, I think that they worked ok for the most recent version, but didn't exist or weren't reliable for some of the older versions that we still supported.
I can back off this a bit and propose a simpler, more direct fix, that doesn't mess with the OpenJPEG portions.
But before I commit to that, did you try this patch on your end and verify that there's something that doesn't work?
There was a problem hiding this comment.
I can back off this a bit and propose a simpler, more direct fix, that doesn't mess with the OpenJPEG portions.
I would agree it's better to separate these.
But before I commit to that, did you try this patch on your end and verify that there's something that doesn't work?
I haven't had the chance, sorry...
PR 4875, switched to using openjph's exported cmake config instead of our own FindOpenJPH.cmake, and in the process also renamed OpenJPH -> openjph to follow their convention. But I botched it, still using the old OPENJPH_LIBRARIES (etc.) veriables instead of fully switching to the correct targets exported from their config. This PR minimally fixes that error, re-enabling use of openjph. Signed-off-by: Larry Gritz <[email protected]>
|
I've fully replaced this PR with a simpler version that just directly addresses the problem: build: fix openjph target use PR 4875, switched to using openjph's exported cmake config instead of But I botched it, still using the old OPENJPH_LIBRARIES (etc.) This PR minimally fixes that error, re-enabling use of openjph. |
lgritz
changed the title
|
LGTM |
PR AcademySoftwareFoundation#4875, switched to using openjph's exported cmake config instead of our own FindOpenJPH.cmake, and in the process also renamed OpenJPH -> openjph to follow their convention. But I botched it, still using the old OPENJPH_LIBRARIES (etc.) variables instead of fully switching to the correct targets exported from their config. This PR minimally fixes that error, re-enabling use of openjph. Fixes AcademySoftwareFoundation#4893 Signed-off-by: Larry Gritz <[email protected]>
2 participants
PR #4875, switched to using openjph's exported cmake config instead of
our own FindOpenJPH.cmake, and in the process also renamed OpenJPH ->
openjph to follow their convention.
But I botched it, still using the old OPENJPH_LIBRARIES (etc.)
variables instead of fully switching to the correct targets exported
from their config.
This PR minimally fixes that error, re-enabling use of openjph.
Fixes #4893