Bug 204657 – [CMake] Add OpenJPEG find module

• Status: RESOLVED
• Resolution: FIXED
• Component: CMake

[[e-mail]]

Required version for Openjpeg is VERSION_LESS "2.2.0", but should be VERSION_GREATER "2.2.0" in order to allow versions like "2.3.0", at file Source/cmake/OptionsGTK.cmake line 378.

This is trying to build 2.27.3 on jhbuild.

[Don Olmstead]

Created attachment 385427 [details]
Patch

[Don Olmstead]

Created attachment 385429 [details]
Patch

[Don Olmstead]

Created attachment 385432 [details]
Patch

[Don Olmstead]

Created attachment 385434 [details]
Patch

[Don Olmstead]

Created attachment 385435 [details]
Patch

[Michael Catanzaro]

Comment on attachment 385435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385435&action=review

> Source/cmake/FindOpenJPEG.cmake:58
> +set(OpenJPEG_VERSION ${PC_OPENJPEG_CFLAGS_VERSION})

This looks phony. ${PC_OPENJPEG_CFLAGS_VERSION}? Shouldn't it be: ${PC_OPENJPEG_VERSION}? If you have a FreeBSD or Linux box handy, might I suggest testing this part? ;)

> Source/cmake/OptionsGTK.cmake:375
>  if (USE_OPENJPEG)
> -    find_package(OpenJPEG)
> -    if (NOT OpenJPEG_FOUND)
> -        message(FATAL_ERROR "libopenjpeg is needed for USE_OPENJPEG.")
> -    endif ()
> -    if ("${OPENJPEG_MAJOR_VERSION}.${OPENJPEG_MINOR_VERSION}.${OPENJPEG_BUILD_VERSION}" VERSION_LESS "2.2.0")
> -        message(FATAL_ERROR "libopenjpeg 2.2.0 is required for USE_OPENJPEG.")
> -    endif ()
> +    find_package(OpenJPEG 2.2.0 REQUIRED)
>  endif ()

The reason we write it out longways, instead of passing REQUIRED to find_package(), is to make the error less-confusing. The intent is for the error message to point to the USE_OPENJPEG build option so that you can just disable that if you don't want OpenJPEG support. But when you pass REQUIRED to find_package(), the error message doesn't indicate that OpenJPEG is optional. So I'd rather keep the long error messages and not use REQUIRED in OptionsGTK.cmake/OptionsWPE.cmake.

BTW I think the bug as reported is invalid. Here we clearly fail if OpenJPEG version is below 2.2.0 and succeed otherwise. Maybe there's something wrong somewhere that I don't see, but I don't think it's here?

Anyway, r+ anyway because I like targets, assuming you rename the change and keep the long error messages instead of switching to find_package(..., REQUIRED).

[Michael Catanzaro]

Comment on attachment 385435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385435&action=review

> Source/cmake/FindOpenJPEG.cmake:89
> +if ("${OpenJPEG_FIND_VERSION}" VERSION_GREATER "${OpenJPEG_VERSION}")
> +    message(FATAL_ERROR "Required version (" ${OpenJPEG_FIND_VERSION} ") is higher than found version (" ${OpenJPEG_VERSION} ")")
> +endif ()

Maybe mention "OpenJPEG" in the error message.

Also: what if it's not used with REQUIRED? Shouldn't be fatal in that case.

[Don Olmstead]

Created attachment 385443 [details]
Patch

[Don Olmstead]

Created attachment 385444 [details]
Patch

[Michael Catanzaro]

Comment on attachment 385444 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 385444 [details]
Patch

Clearing flags on attachment: 385444

Committed r253406: <https://trac.webkit.org/changeset/253406>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.