Bug 180949 - [GTK][WPE] Conditionalize libTASN1 use behind ENABLE_SUBTLE_CRYPTO in the CMake files
Summary: [GTK][WPE] Conditionalize libTASN1 use behind ENABLE_SUBTLE_CRYPTO in the CMa...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Other
Hardware: Other Linux
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on: 181012
Blocks:
  Show dependency treegraph
 
Reported: 2017-12-18 14:11 PST by Timothy Hatcher
Modified: 2017-12-19 16:47 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.78 KB, patch)
2017-12-18 14:13 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2017-12-18 14:11:55 PST
When ENABLE_SUBTLE_CRYPTO is disable the include paths and libraries should not include missing LIBTASN1 references.
Comment 1 Timothy Hatcher 2017-12-18 14:13:53 PST
Created attachment 329682 [details]
Patch
Comment 2 WebKit Commit Bot 2017-12-18 20:09:14 PST
Comment on attachment 329682 [details]
Patch

Clearing flags on attachment: 329682

Committed r226094: <https://trac.webkit.org/changeset/226094>
Comment 3 WebKit Commit Bot 2017-12-18 20:09:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 4 Radar WebKit Bug Importer 2017-12-18 20:10:17 PST
<rdar://problem/36123388>
Comment 5 Michael Catanzaro 2017-12-19 09:10:31 PST
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.
Comment 6 Michael Catanzaro 2017-12-19 09:16:56 PST
(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.
Comment 7 Timothy Hatcher 2017-12-19 09:21:23 PST
(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.
Comment 8 Michael Catanzaro 2017-12-19 09:25:28 PST
(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.
Comment 9 Timothy Hatcher 2017-12-19 09:29:39 PST
(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.
Comment 10 Timothy Hatcher 2017-12-19 14:00:56 PST
(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.
Comment 11 Michael Catanzaro 2017-12-19 15:00:12 PST
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.