Bug 180949 – [GTK][WPE] Conditionalize libTASN1 use behind ENABLE_SUBTLE_CRYPTO in the CMake files

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Timothy Hatcher]

When ENABLE_SUBTLE_CRYPTO is disable the include paths and libraries should not include missing LIBTASN1 references.

[Timothy Hatcher]

Created attachment 329682 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 329682 [details]
Patch

Clearing flags on attachment: 329682

Committed r226094: <https://trac.webkit.org/changeset/226094>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36123388>

[Michael Catanzaro]

This actually is not the best way to do it. Two problems:

1) The check is still done unconditionally in OptionsWPE.cmake, even though ENABLE_SUBTLE_CRYPTO is no longer mandatory for WPE. That's wrong. So it's currently impossible to build without Libtasn1, and the added conditionals in PlatformWPE.cmake are useless as they'll never be reached. The check for Libtasn1 should instead be moved down and placed inside an if (ENABLE_SUBTLE_CRYPTO) block in OptionsWPE.cmake, then it will really be possible to build without.

2) It's simpler to unconditionally add _LIBRARIES and _INCLUDE_DIRS in Platform*.cmake. If the checks are made conditional in Options*.cmake, then (I think) the _LIBRARIES and _INCLUDE_DIRS variables will expand to empty strings, so adding conditionals in Platform*.cmake should not be needed.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> 2) It's simpler to unconditionally add _LIBRARIES and _INCLUDE_DIRS in
> Platform*.cmake. If the checks are made conditional in Options*.cmake, then
> (I think) the _LIBRARIES and _INCLUDE_DIRS variables will expand to empty
> strings, so adding conditionals in Platform*.cmake should not be needed.

Although it might be a bit confusing to intentionally use undefined variables, it is IMO good style in this case, because the Platform*.cmake files will otherwise become a mess of unneeded conditionals for different build configurations.

It's unfortunate that the existing code does not always follow this style. E.g.:

# Source/WebCore/PlatformGTK.cmake
if (ENABLE_WAYLAND_TARGET)
    list(APPEND WebCore_SYSTEM_INCLUDE_DIRECTORIES
        ${WAYLAND_INCLUDE_DIRS}
    )
    list(APPEND WebCore_LIBRARIES
        ${WAYLAND_LIBRARIES}
    )
endif ()
 
I believe that condition is not needed, because ${WAYLAND_INCLUDE_DIRS} and ${WAYLAND_LIBRARIES} are only defined if ENABLE_WAYLAND_TARGET is true; they can be appended to WebCore_SYSTEM_INCLUDE_DIRECTORIES and WebCore_LIBRARIES unconditionally.

[Timothy Hatcher]

(In reply to Michael Catanzaro from comment #6)
> (In reply to Michael Catanzaro from comment #5)
> > 2) It's simpler to unconditionally add _LIBRARIES and _INCLUDE_DIRS in
> > Platform*.cmake. If the checks are made conditional in Options*.cmake, then
> > (I think) the _LIBRARIES and _INCLUDE_DIRS variables will expand to empty
> > strings, so adding conditionals in Platform*.cmake should not be needed.
> 
> Although it might be a bit confusing to intentionally use undefined
> variables, it is IMO good style in this case, because the Platform*.cmake
> files will otherwise become a mess of unneeded conditionals for different
> build configurations.
> 
> It's unfortunate that the existing code does not always follow this style.
> E.g.:
> 
> # Source/WebCore/PlatformGTK.cmake
> if (ENABLE_WAYLAND_TARGET)
>     list(APPEND WebCore_SYSTEM_INCLUDE_DIRECTORIES
>         ${WAYLAND_INCLUDE_DIRS}
>     )
>     list(APPEND WebCore_LIBRARIES
>         ${WAYLAND_LIBRARIES}
>     )
> endif ()
>  
> I believe that condition is not needed, because ${WAYLAND_INCLUDE_DIRS} and
> ${WAYLAND_LIBRARIES} are only defined if ENABLE_WAYLAND_TARGET is true; they
> can be appended to WebCore_SYSTEM_INCLUDE_DIRECTORIES and WebCore_LIBRARIES
> unconditionally.

I was hitting a linker or build error (can't recall which, this was a while ago) when they were unconditional. I wouldn't have touched this otherwise. Maybe just a side effect of our platform at Tesla?

I was just following the style of similar conditionals. If you feel this should be cleaned up for all cases, then let track that separately. I don't have the cycles to do it though.

[Michael Catanzaro]

(In reply to Timothy Hatcher from comment #7)
> I was hitting a linker or build error (can't recall which, this was a while
> ago) when they were unconditional. I wouldn't have touched this otherwise.
> Maybe just a side effect of our platform at Tesla?

I don't know.

I don't expect you to clean up all cases, just Libtasn1, since that's what this patch is about. I would revert this patch and change the unconditional check for Libtasn1 in OptionsWPE.cmake to look more like the check in OptionsGTK.cmake instead.

[Timothy Hatcher]

(In reply to Michael Catanzaro from comment #8)
> (In reply to Timothy Hatcher from comment #7)
> > I was hitting a linker or build error (can't recall which, this was a while
> > ago) when they were unconditional. I wouldn't have touched this otherwise.
> > Maybe just a side effect of our platform at Tesla?
> 
> I don't know.
> 
> I don't expect you to clean up all cases, just Libtasn1, since that's what
> this patch is about. I would revert this patch and change the unconditional
> check for Libtasn1 in OptionsWPE.cmake to look more like the check in
> OptionsGTK.cmake instead.

Okay, I'll do that.

[Timothy Hatcher]

(In reply to Michael Catanzaro from comment #8)
> (In reply to Timothy Hatcher from comment #7)
> > I was hitting a linker or build error (can't recall which, this was a while
> > ago) when they were unconditional. I wouldn't have touched this otherwise.
> > Maybe just a side effect of our platform at Tesla?
> 
> I don't know.
> 
> I don't expect you to clean up all cases, just Libtasn1, since that's what
> this patch is about. I would revert this patch and change the unconditional
> check for Libtasn1 in OptionsWPE.cmake to look more like the check in
> OptionsGTK.cmake instead.

OptionsGTK.cmake and OptionsWPE.cmake are identical in how they require the Libtasn1 package. I don't know what you are wanting to do.

[Michael Catanzaro]

Oops, I was looking at my stale local checkout! Indeed, no changes are required there.

In that case, I'm tempted to say that all we need to do is revert this patch. I don't think reverting it should cause any link errors, as explained above. If it does, I'd like to see the error.