Bug 218069 – Move some initialization code from top-level CMakeLists.txt to WebKitCommon.cmake

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Keith Rollin]

Moving this initialization code into a central location allows other top-level CMakeLists.txt files to include WebKitCommon.cmake and get that same initialization.

[Radar WebKit Bug Importer]

<rdar://problem/70556952>

[Keith Rollin]

Created attachment 412062 [details]
Patch

[Fujii Hironori]

Comment on attachment 412062 [details]
Patch

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

> Source/cmake/WebKitCommon.cmake:35
> +        Efl

Efl port was removed.

> CMakeLists.txt:2
>  # Determine CMake version and build type.

This file no longer determines build type. This comment becomes invalid.

[Fujii Hironori]

(In reply to Keith Rollin from comment #0)
> Moving this initialization code into a central location allows other
> top-level CMakeLists.txt files to include WebKitCommon.cmake and get that
> same initialization.

I don't know the reason why you want to include WebKit's CMake
files from the outside of WebKit, but basically it is a bad idea.
It means that those CMake files are public API.

For example, Apple Safari code is (was?) including WTF headers,
so some changes were reverted because it breaks internal Apple
builds.

[Don Olmstead]

Comment on attachment 412062 [details]
Patch

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

WebKitCommon is included in every Source/$<lib>/CMakeLists.txt specifically to handle the internal AppleWin build so this change will most likely cause breakage there.

I know whatever you're up to Keith will encourage Apple to switch over to CMake which is a great thing but not being able to see what this private project is up to makes me a bit nervous. I really feel that WebKit should be built as an External project and maybe we can come up with some ways to have a CMake config or something for you to consume see https://gitlab.kitware.com/cmake/community/-/wikis/doc/tutorials/How-to-create-a-ProjectConfig.cmake-file

> CMakeLists.txt:15
> +set(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/Source/cmake")

Is anyone ever passing in a CMAKE_MODULE_PATH because if so this seems like it'll nuke it which is bad.

[Konstantin Tokarev]

(In reply to Don Olmstead from comment #5)
> maybe we can come up with some ways to have
> a CMake config or something for you to consume see
> https://gitlab.kitware.com/cmake/community/-/wikis/doc/tutorials/How-to-
> create-a-ProjectConfig.cmake-file

This is definitely the right way to use WebKit in other cmake projects. Such config files can even be installed system-wide, so using something like find_package(WebKit) would automatically find and include them in user's project.

[Michael Catanzaro]

Comment on attachment 412062 [details]
Patch

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

>> CMakeLists.txt:15
>> +set(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/Source/cmake")
> 
> Is anyone ever passing in a CMAKE_MODULE_PATH because if so this seems like it'll nuke it which is bad.

Yeah, we should probably append rather than overwrite it. But it's also been there forever (just higher in the file).

[Fujii Hironori]

(In reply to Konstantin Tokarev from comment #6)
> This is definitely the right way to use WebKit in other cmake projects. 

It isn't the only right way.
Another right way to include other projects is using add_subdirectory.
https://cmake.org/cmake/help/latest/command/add_subdirectory.html

[Fujii Hironori]

Comment on attachment 412062 [details]
Patch

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

>>> CMakeLists.txt:15
>>> +set(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/Source/cmake")
>> 
>> Is anyone ever passing in a CMAKE_MODULE_PATH because if so this seems like it'll nuke it which is bad.
> 
> Yeah, we should probably append rather than overwrite it. But it's also been there forever (just higher in the file).

WebKit doesn't need other CMAKE_MODULE_PATH or the parent's CMAKE_MODULE_PATH.
WebKit is using only WebKit's cmake files and builtin ones.
Appending CMAKE_MODULE_PATH can break WebKit builds by finding out an incorrect cmake file in the parent project.

[Konstantin Tokarev]

(In reply to Fujii Hironori from comment #8)
> (In reply to Konstantin Tokarev from comment #6)
> > This is definitely the right way to use WebKit in other cmake projects. 
> 
> It isn't the only right way.
> Another right way to include other projects is using add_subdirectory.
> https://cmake.org/cmake/help/latest/command/add_subdirectory.html

add_subdirectory should never be used for including projects which you don't fully control, external projects should use config files or find modules

[Konstantin Tokarev]

(In reply to Fujii Hironori from comment #9)
> Comment on attachment 412062 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412062&action=review
> 
> >>> CMakeLists.txt:15
> >>> +set(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/Source/cmake")
> >> 
> >> Is anyone ever passing in a CMAKE_MODULE_PATH because if so this seems like it'll nuke it which is bad.
> > 
> > Yeah, we should probably append rather than overwrite it. But it's also been there forever (just higher in the file).
> 
> WebKit doesn't need other CMAKE_MODULE_PATH or the parent's
> CMAKE_MODULE_PATH.
> WebKit is using only WebKit's cmake files and builtin ones.
> Appending CMAKE_MODULE_PATH can break WebKit builds by finding out an
> incorrect cmake file in the parent project.

While I don't see a good use case for passing custom CMAKE_MODULE_PATH, I think it's not bad to use append as well. If user specifies CMAKE_MODULE_PATH for some reason, it's their responsibility if something gets broken as a result.

[Keith Rollin]

(In reply to Fujii Hironori from comment #4)
> (In reply to Keith Rollin from comment #0)
> > Moving this initialization code into a central location allows other
> > top-level CMakeLists.txt files to include WebKitCommon.cmake and get that
> > same initialization.
> 
> I don't know the reason why you want to include WebKit's CMake
> files from the outside of WebKit, but basically it is a bad idea.
> It means that those CMake files are public API.
> 
> For example, Apple Safari code is (was?) including WTF headers,
> so some changes were reverted because it breaks internal Apple
> builds.

I hear you and understand your concern. However, my take is that what I'm doing is a necessary risk. The external CMake files I'm creating reference things like DERIVED_SOURCES_DIR and WEBKIT_COPY_FILES. I don't remember the details right now, but at one point I also needed to have the CPU-related definitions available. If I include WebKitCommon as I'm doing now, then I can use those as is, at the risk of breaking if an incompatible change to the WebKit CMake files are checked in. On the other hand, I could copy the definitions of DERIVED_SOURCES_DIR and WEBKIT_COPY_FILES and the CPU-sniffing code into my external project, but then have the burden of keeping them in sync with the WebKit copies, and still suffer the possibility of breaking if an incompatible change is checked in.

Those are the two alternatives I can see. Do you see a better approach?

Keep in mind that I have the requirement of doing some setup before performing the WebKit build. WebKit can be extended by making some files available and then having the WebKit build system discover them and incorporate them. That's why I have an external CMake file that performs this setup, performs the WebKit build, and then continues on with some downstream builds based on the built WebKit.
I hear you and understand your concern. However, my take is that what I'm doing is a necessary risk. The external CMake files I'm creating reference things like DERIVED_SOURCES_DIR and WEBKIT_COPY_FILES. I don't remember the details right now, but at one point I also needed to have the CPU-related definitions available. If I include WebKitCommon as I'm doing now, then I can use those as is, at the risk of breaking if an incompatible change to the WebKit CMake files are checked in. On the other hand, I could copy the definitions of DERIVED_SOURCES_DIR and WEBKIT_COPY_FILES and the CPU-sniffing code into my external project, but then have the burden of keeping them in sync with the WebKit copies, and still suffer the possibility of breaking if an incompatible change is checked in.

Those are the two alternatives I can see. Do you see a better approach?

Keep in mind that I have the requirement of doing some setup before performing the WebKit build. WebKit can be extended by making some files available and then having the WebKit build system discover them and incorporate them. That's why I have an external CMake file that performs this setup, performs the WebKit build, and then continues on with some downstream builds based on the built WebKit.

[Keith Rollin]

(In reply to Don Olmstead from comment #5)
> Comment on attachment 412062 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412062&action=review
> 
> WebKitCommon is included in every Source/$<lib>/CMakeLists.txt specifically
> to handle the internal AppleWin build so this change will most likely cause
> breakage there.

What kind of breakage do you see happening? The "win" bubble is green above; what other types of builds are being performed that will break?

> 
> I know whatever you're up to Keith will encourage Apple to switch over to
> CMake which is a great thing but not being able to see what this private
> project is up to makes me a bit nervous. I really feel that WebKit should be
> built as an External project and maybe we can come up with some ways to have
> a CMake config or something for you to consume see
> https://gitlab.kitware.com/cmake/community/-/wikis/doc/tutorials/How-to-
> create-a-ProjectConfig.cmake-file

I've been trying to understand this example (remember that I'm a cmake beginner) but can't see that it helps my case. See my Comment #12. I don't see how ExternalProject or that article helps me.

> 
> > CMakeLists.txt:15
> > +set(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/Source/cmake")
> 
> Is anyone ever passing in a CMAKE_MODULE_PATH because if so this seems like
> it'll nuke it which is bad.

This is the same line that already existed in the file; I just moved it out of the block of code I moved to WebKitCommon.

For a little bit, I did have a guard around it so that it didn't wipe out any previous CMAKE_MODULE_PATH (I didn't realize that it could be added on to). I had defined CMAKE_MODULE_PATH in my external CMakeLists.txt file so that it could include(WebKitCommon), and worried about it getting redefined in WebKit's CMakeLists.txt file. But both definitions pointed to the same directory, so I was OK with CMAKE_MODULE_PATH getting nuked.

[Keith Rollin]

To be clear, even though there's an r+, I'll hold off landing this patch until there's agreement on the approach.

[Fujii Hironori]

Comment on attachment 412062 [details]
Patch

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

>>>>>> CMakeLists.txt:15
>>>>>> +set(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/Source/cmake")
>>>>> 
>>>>> Is anyone ever passing in a CMAKE_MODULE_PATH because if so this seems like it'll nuke it which is bad.
>>>> 
>>>> Yeah, we should probably append rather than overwrite it. But it's also been there forever (just higher in the file).
>>> 
>>> WebKit doesn't need other CMAKE_MODULE_PATH or the parent's CMAKE_MODULE_PATH.
>>> WebKit is using only WebKit's cmake files and builtin ones.
>>> Appending CMAKE_MODULE_PATH can break WebKit builds by finding out an incorrect cmake file in the parent project.
>> 
>> While I don't see a good use case for passing custom CMAKE_MODULE_PATH, I think it's not bad to use append as well. If user specifies CMAKE_MODULE_PATH for some reason, it's their responsibility if something gets broken as a result.
> 
> This is the same line that already existed in the file; I just moved it out of the block of code I moved to WebKitCommon.
> 
> For a little bit, I did have a guard around it so that it didn't wipe out any previous CMAKE_MODULE_PATH (I didn't realize that it could be added on to). I had defined CMAKE_MODULE_PATH in my external CMakeLists.txt file so that it could include(WebKitCommon), and worried about it getting redefined in WebKit's CMakeLists.txt file. But both definitions pointed to the same directory, so I was OK with CMAKE_MODULE_PATH getting nuked.

CMake variabls have Directory Scope. You need to use PARENT_SCOPE to nuke the parent directory's CMAKE_MODULE_PATH.
set(CMAKE_MODULE_PATH ... PARENT_SCOPE)

https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#cmake-language-variables
https://cmake.org/cmake/help/latest/command/set.html

[Fujii Hironori]

There seem two issues of discussion.

1. add_subdirectory vs. ExternalProject
2. Using WebKit internal CMake files from the outside of WebKit

I think both issues are trivial, we can choose another the
approach if we'll have a real problem. We should go forward in
the direction Keith chose.

ExternalProject — CMake Documentation
https://cmake.org/cmake/help/latest/module/ExternalProject.html

[Fujii Hironori]

(In reply to Keith Rollin from comment #13)
> What kind of breakage do you see happening? The "win" bubble is green above;
> what other types of builds are being performed that will break?

OpenSource ApplwWin EWS and OpenSource AppleWin buildbot builder don't do the infamous per-directory-builds.
Only ApplwWin Apple Internal build system does the per-directory-builds.

But, I guess this change won't break it.

[Don Olmstead]

(In reply to Fujii Hironori from comment #17)
> (In reply to Keith Rollin from comment #13)
> > What kind of breakage do you see happening? The "win" bubble is green above;
> > what other types of builds are being performed that will break?
> 
> OpenSource ApplwWin EWS and OpenSource AppleWin buildbot builder don't do
> the infamous per-directory-builds.
> Only ApplwWin Apple Internal build system does the per-directory-builds.
> 
> But, I guess this change won't break it.

Its my understanding that Apple's internal build for WebKit checks out a single Source directory, so WTF/JavaScriptCore/etc, and builds that then passes this along to the next builder. There is an internal AppleWin build that behaves the same way but uses Visual Studio projects and CMake.

You can see in each subdirectory of Source there's a ${LIB_NAME}.vcxproj that contains a Visual Studio project that invokes CMake https://github.com/WebKit/webkit/blob/master/Source/WebCore/WebCore.vcxproj/WebCore.proj#L31 which actually does the build. It acts on the CMakeLists.txt on each subdirectory of Source which is why those include()'s at the top of each Source/${LIB_NAME}/CMakeLists.txt exist.

You can talk to Brent, Per Arne, or Alex about the internal build more. I just know this much because I've accidentally broken it so many times 😅 and I would be paranoid that this change would do something there.

[Alex Christensen]

I don't think this change will break the internal build.

[EWS]

Committed r268991: <https://trac.webkit.org/changeset/268991>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412062 [details].