Bug 198046

Created attachment 370264 [details] Patch

Comment on attachment 370264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370264&action=review Now we're cooking. > Source/bmalloc/CMakeLists.txt:163 > +add_library(WebKit::bmalloc ALIAS bmallocFramework) Why is this alias necessary or helpful?

Comment on attachment 370264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370264&action=review > Source/bmalloc/CMakeLists.txt:39 > +set(bmalloc_PUBLIC_HEADERS It's unfortunate....

Especially since sources are being listed in Sources.txt, having headers in the CMakeLists.txt isn't great.

(In reply to Michael Catanzaro from comment #2) > Comment on attachment 370264 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370264&action=review > > Now we're cooking. > > > Source/bmalloc/CMakeLists.txt:163 > > +add_library(WebKit::bmalloc ALIAS bmallocFramework) > > Why is this alias necessary or helpful? I was looking at how CMake does stuff with their own Find modules when it creates a target. It wants to namespace things with the Find* name so thats why we got ICU::data, ICU::uc, ICU::i1n8. So hypothetically if we had a FindWebKit we would have things like WebKit::WTF WebKit::JavaScriptCore etc. (In reply to Michael Catanzaro from comment #3) > Comment on attachment 370264 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370264&action=review > > > Source/bmalloc/CMakeLists.txt:39 > > +set(bmalloc_PUBLIC_HEADERS > > It's unfortunate.... That's CMake though. If we don't list them and anyone adds anything or removes it it can potentially break the build. 🤷

(In reply to Don Olmstead from comment #5) > I was looking at how CMake does stuff with their own Find modules when it > creates a target. It wants to namespace things with the Find* name so thats > why we got ICU::data, ICU::uc, ICU::i1n8. So hypothetically if we had a > FindWebKit we would have things like WebKit::WTF WebKit::JavaScriptCore etc. No, I mean: what's the point of the bmallocFramework alias?

(In reply to Michael Catanzaro from comment #6) > (In reply to Don Olmstead from comment #5) > > I was looking at how CMake does stuff with their own Find modules when it > > creates a target. It wants to namespace things with the Find* name so thats > > why we got ICU::data, ICU::uc, ICU::i1n8. So hypothetically if we had a > > FindWebKit we would have things like WebKit::WTF WebKit::JavaScriptCore etc. > > No, I mean: what's the point of the bmallocFramework alias? Oh sorry. So the interface target is to combine the bmalloc library and the copying of headers. WEBKIT_COPY_FILES just creates a target that copies the files. It is not dependent on anything. So the bmallocFramework is the combination of building bmalloc and copying the headers. It will also end up propagating the include directory for the bmalloc headers. Then the WebKit::bmalloc alias just fits with CMake conventions.

Comment on attachment 370264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370264&action=review >Then the WebKit::bmalloc alias just fits with CMake conventions. AFAIK CMake convention is to use namespace only for external targets, and when exporting internal targets via install(EXPORT) >>> Source/bmalloc/CMakeLists.txt:163 >>> +add_library(WebKit::bmalloc ALIAS bmallocFramework) >> >> Why is this alias necessary or helpful? > > I was looking at how CMake does stuff with their own Find modules when it creates a target. It wants to namespace things with the Find* name so thats why we got ICU::data, ICU::uc, ICU::i1n8. So hypothetically if we had a FindWebKit we would have things like WebKit::WTF WebKit::JavaScriptCore etc. > > (In reply to Michael Catanzaro from comment #3) This feature is supposed to work in a different way. When install(EXPORT) command is used to create cmake config files with exported targets, it's given NAMESPACE argument, e.g. it can be "WebKit::". Other project which uses find_pakage(WebKit) will get these targets inside namespace, which doesn't affect their names in WebKit project itself.

Comment on attachment 370264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370264&action=review >>> Source/bmalloc/CMakeLists.txt:39 >>> +set(bmalloc_PUBLIC_HEADERS >> >> It's unfortunate.... > > That's CMake though. If we don't list them and anyone adds anything or removes it it can potentially break the build. 🤷 AFAICS, the only header which is used outside of bmalloc implementation is bmalloc.h. We certainly don't need to copy anything else

> Oh sorry. So the interface target is to combine the bmalloc library and the > copying of headers. WEBKIT_COPY_FILES just creates a target that copies the > files. It is not dependent on anything. So the bmallocFramework is the > combination of building bmalloc and copying the headers. It will also end up > propagating the include directory for the bmalloc headers. I think this approach is not optimal. We should switch to a different pattern: 1) target FrameworkName_GeneratedHeaders depends on all targets which generate files; 2) target FrameworkName_CopyHeaders depends on FrameworkName_GeneratedHeaders (if it exists) 3) main target FrameworkName depends on FrameworkName_CopyHeaders Benefits: 1) more intuitive target names; 2) in case of bmalloc there are no generated headers so we'll have two targets instead of three

(In reply to Konstantin Tokarev from comment #9) > Comment on attachment 370264 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370264&action=review > > >>> Source/bmalloc/CMakeLists.txt:39 > >>> +set(bmalloc_PUBLIC_HEADERS > >> > >> It's unfortunate.... > > > > That's CMake though. If we don't list them and anyone adds anything or removes it it can potentially break the build. 🤷 > > AFAICS, the only header which is used outside of bmalloc implementation is > bmalloc.h. We certainly don't need to copy anything else bmalloc.h includes many other things. Honestly it would be nicer if headers were organized in a way that we could tell when something is public vs private. (In reply to Konstantin Tokarev from comment #10) > > Oh sorry. So the interface target is to combine the bmalloc library and the > > copying of headers. WEBKIT_COPY_FILES just creates a target that copies the > > files. It is not dependent on anything. So the bmallocFramework is the > > combination of building bmalloc and copying the headers. It will also end up > > propagating the include directory for the bmalloc headers. > > I think this approach is not optimal. We should switch to a different > pattern: > 1) target FrameworkName_GeneratedHeaders depends on all targets which > generate files; > 2) target FrameworkName_CopyHeaders depends on > FrameworkName_GeneratedHeaders (if it exists) > 3) main target FrameworkName depends on FrameworkName_CopyHeaders > > Benefits: 1) more intuitive target names; 2) in case of bmalloc there are no > generated headers so we'll have two targets instead of three Another reason I went with the WebKit::${framework} style was that for the AppleWin internal build, and a hypothetical CMake Apple build that rules them all, there would be a cmake file that would create an IMPORTED WebKit::${framework}. Outside of the Source/ directory being built WebKit::${framework} is how things should be referred to. The other Framework targets youre mentioning would just be internal.

(In reply to Don Olmstead from comment #11) > (In reply to Konstantin Tokarev from comment #9) > > AFAICS, the only header which is used outside of bmalloc implementation is > > bmalloc.h. We certainly don't need to copy anything else > > bmalloc.h includes many other things. Sorry, I've missed this moment. > Honestly it would be nicer if headers were organized in a way that we could tell when something is public vs private. Yes. Alternatively, I think we could extract header properties automatically from pbxproj files with script. CMake target which runs script would depend on respective pbxproj, so file list will be re-generated appropriately (as opposed to unreliable cmake globs) > > (In reply to Konstantin Tokarev from comment #10) > > > Oh sorry. So the interface target is to combine the bmalloc library and the > > > copying of headers. WEBKIT_COPY_FILES just creates a target that copies the > > > files. It is not dependent on anything. So the bmallocFramework is the > > > combination of building bmalloc and copying the headers. It will also end up > > > propagating the include directory for the bmalloc headers. > > > > I think this approach is not optimal. We should switch to a different > > pattern: > > 1) target FrameworkName_GeneratedHeaders depends on all targets which > > generate files; > > 2) target FrameworkName_CopyHeaders depends on > > FrameworkName_GeneratedHeaders (if it exists) > > 3) main target FrameworkName depends on FrameworkName_CopyHeaders > > > > Benefits: 1) more intuitive target names; 2) in case of bmalloc there are no > > generated headers so we'll have two targets instead of three > > Another reason I went with the WebKit::${framework} style was that for the > AppleWin internal build, and a hypothetical CMake Apple build that rules > them all, there would be a cmake file that would create an IMPORTED > WebKit::${framework}. Outside of the Source/ directory being built > WebKit::${framework} is how things should be referred to. The other > Framework targets youre mentioning would just be internal. On a second thought, it indeed makes sense to use WebKit::${framework} naming. While it contradicts what I've written above, it indeed allows to have IMPORTED and non-IMPORTED targets with the same names. However, I think they should replace ${framework}Framework targets instead of aliasing them. This won't affect AppleWin build because when building WebKit as separate projects, project that builds some target doesn't need to import it, so there won't be target name clashes. WebKit::bmalloc also looks a bit more pleasant to me than bmallocFramework - as I said before, use of word "Framework" here is somewhat controversial, while WebKit:: obviously isn't.

So, my proposal on target changes transforms to following: 1) target FrameworkName_GeneratedHeaders depends on all targets which generate files; 2) target FrameworkName_CopyHeaders depends on FrameworkName_GeneratedHeaders (if it exists) 3) target WebKit::FrameworkName replaces old FrameworkName target and additionally depends on FrameworkName_CopyHeaders (and does other stuff which is done by FrameworkName now) I think it's better to do such change sooner or better, before we haven't drown in the flood of dummy targets with forwared interfaces. BTW, for dummy target which only handles dependencies it's possible to use add_custom_target(name) instead of introducing INTERFACE target

Created attachment 370360 [details] Patch

Comment on attachment 370360 [details] Patch Attachment 370360 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12252292 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla apiTests

Comment on attachment 370360 [details] Patch Attachment 370360 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12254734 New failing tests: js/dom/custom-constructors.html

Created attachment 370387 [details] Archive of layout-test-results from ews213 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews213 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Modified the patch to do the following. bmalloc_CopyHeaders - copies the headers required for using bmalloc bmalloc_Internal - this is an INTERFACE target that links bmalloc, depends on bmalloc_CopyHeaders and propagates bmalloc_FRAMEWORK_HEADERS_DIR to dependent projects. WebKit::bmalloc - this is an ALIAS target of bmalloc_Internal that namespaces bmalloc under WebKit:: unfortunately CMake restricts namespaced targets to ALIAS and IMPORTED libraries so this is necessary. Note that in CMake terms an INTERFACE and an ALIAS target are internal to CMake. In Visual Studio for example there would not be bmalloc_Iternal project file. The reason for the ALIAS target, which is just the INTERFACE target, is purely to present the final WebKit::framework as if it were an installed project. This is to help support Apple internal builds where each subdirectory of Source is checked out in isolation and built. This is a bit confusing but I think it'll make more sense once I get to WTF. For this case there would be a target definition that would create an IMPORTED WebKit::framework target. Since AppleWin does not use bmalloc this is not present in this patch but would be in a patch for WTF. Now if we follow this pattern here's what JavaScriptCore would look like. JavaScriptCore_CopyHeaders - copies the public framework headers JavaScriptCore_CopyPrivateHeaders - copies the private framework headers JavaScriptCore_Internal - an INTERFACE target that links JavaScriptCore, depends on JavaScriptCore_CopyHeaders and JavaScriptCore_CopyPrivateHeaders, and propagates JavaScriptCore_FRAMEWORK_HEADERS_DIR and JavaScriptCore_PRIVATE_FRAMEWORK_HEADERS_DIR. JavaScriptCore depends on JavaScriptCore_CopyHeaders because there are references to public JavaScriptCore headers when building that library. JavaScriptCore does not depend on JavaScriptCore_CopyPrivateHeaders because that's not actually needed for building JavaScriptCore. We don't actually want the directory the private headers end up in to be accessible when building JavaScriptCore. There's a long history of improper includes that have caused problems. When using WEBKIT_MAKE_FORWARDING_HEADERS it creates a dependency on the framework for the headers. This can also hurt parallelization of the build. As is when you start up a build WTF is dependent on copying its headers so the first step is always to copy the headers. By removing that then both copying and building of WTF can be done in parallel. WEBKIT_COPY_FILES expects you to use add_dependencies to set up everything so the build can be smarter about building the targets. Does this seem like an agreeable future? I'm ready to do this with the rest of the libraries.

(In reply to Don Olmstead from comment #18) > Note that in CMake terms an INTERFACE and an ALIAS target are internal to > CMake. In Visual Studio for example there would not be bmalloc_Iternal > project file. Doesn't CMake create Ninja target for INTERFACE target? Even if having extra INTERFACE target has no side effects outside of CMake, this solution still has problems: 1) Platform* files can directly modify properties of library/executable targets now without modifying platform-independent project parts. Some of these properties may affect other targets, so care must be taken to copy all PUBLIC/INTERFACE properties of library/executable target to its INTERFACE target. Sounds like reinventing CMake 2) I think architecture without INTERFACE target is easier to understand, and having dedicated target for all generated files which belong to some library/executable will improve project structure. In particular, having dedicated lists of such generated files instead of appending them directly to target sources 3) (minor) Having CopyHeaders stage after linking stage reduces build parallelism as compared to architecture when CopyHeaders depend on GeneratedFiles

(In reply to Konstantin Tokarev from comment #19) > (In reply to Don Olmstead from comment #18) > > Note that in CMake terms an INTERFACE and an ALIAS target are internal to > > CMake. In Visual Studio for example there would not be bmalloc_Iternal > > project file. > > Doesn't CMake create Ninja target for INTERFACE target? > > Even if having extra INTERFACE target has no side effects outside of CMake, > this solution still has problems: > 1) Platform* files can directly modify properties of library/executable > targets now without modifying platform-independent project parts. Some of > these properties may affect other targets, so care must be taken to copy all > PUBLIC/INTERFACE properties of library/executable target to its INTERFACE > target. Sounds like reinventing CMake The INTERFACE target links the framework so any PUBLIC include directories or targets will be propagated. > 2) I think architecture without INTERFACE target is easier to understand, > and having dedicated target for all generated files which belong to some > library/executable will improve project structure. In particular, having > dedicated lists of such generated files instead of appending them directly > to target sources I'm not sure I totally follow the problem here. For example the target created for WEBKIT_COPY_HEADERS has a list and a destination. For JSC the public header directory should end up in one place while the private header directory should end up in another place. This matches Apple's XCode setup. There is no reason for JSC to wait on copying headers before it builds. I know you don't like that we're enumerating all the headers. It is a long list but I don't see how we get around that. Listing files is a CMakeism and using globbing caused all sorts of problems. I'm open to finding a better way but the listing of headers is pretty much done at this point. > 3) (minor) Having CopyHeaders stage after linking stage reduces build > parallelism as compared to architecture when CopyHeaders depend on > GeneratedFiles I haven't done any timing checks but when I moved to WEBKIT_COPY_HEADERS I saw a lot more interspersing of copying and building source files. It is most likely a win on build time but I don't know how much of one.

(In reply to Don Olmstead from comment #20) > I know you don't like that we're enumerating all the headers. It is a long > list but I don't see how we get around that. Listing files is a CMakeism and > using globbing caused all sorts of problems. I'm open to finding a better > way but the listing of headers is pretty much done at this point. Listing *source* files is a CMakeism; listing *headers* is unusual and only required because we have weird requirements for copying headers around to different places during the build....

(In reply to Michael Catanzaro from comment #21) > (In reply to Don Olmstead from comment #20) > > I know you don't like that we're enumerating all the headers. It is a long > > list but I don't see how we get around that. Listing files is a CMakeism and > > using globbing caused all sorts of problems. I'm open to finding a better > > way but the listing of headers is pretty much done at this point. > > Listing *source* files is a CMakeism; listing *headers* is unusual and only > required because we have weird requirements for copying headers around to > different places during the build.... We don't have a clear differentiation between what are internal includes and what are public includes that would make globbing easier so if you actually did want an INSTALL step you'd still be listing things out.

>The INTERFACE target links the framework so any PUBLIC include directories or targets will be propagated. Oh, I didn't realize that. Then it looks sensible. I think bmalloc_Internal should be renamed to bmalloc_PostBuild, as it simulates behavior of add_custom_command(POST_BUILD)

Created attachment 370453 [details] Patch Use bmalloc_PostBuild

Comment on attachment 370453 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370453&action=review > Source/bmalloc/CMakeLists.txt:159 > +target_include_directories(bmalloc_PostBuild INTERFACE ${bmalloc_FRAMEWORK_HEADERS_DIR}) I think it would be cleaner to set ${bmalloc_FRAMEWORK_HEADERS_DIR} as INTERFACE include directory on bmalloc target. It will be automatically forwarded to bmalloc_PostBuild alongside with other transitive properties, and it will be clear that the only roles of WebKit::bmalloc target is its dependency on CopyHeaders

(In reply to Don Olmstead from comment #22) > We don't have a clear differentiation between what are internal includes and > what are public includes that would make globbing easier so if you actually > did want an INSTALL step you'd still be listing things out. Of course, but public includes are few and far between, probably 0.1% of headers in WebKit.

(In reply to Konstantin Tokarev from comment #25) > Comment on attachment 370453 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370453&action=review > > > Source/bmalloc/CMakeLists.txt:159 > > +target_include_directories(bmalloc_PostBuild INTERFACE ${bmalloc_FRAMEWORK_HEADERS_DIR}) > > I think it would be cleaner to set ${bmalloc_FRAMEWORK_HEADERS_DIR} as > INTERFACE include directory on bmalloc target. It will be automatically > forwarded to bmalloc_PostBuild alongside with other transitive properties, > and it will be clear that the only roles of WebKit::bmalloc target is its > dependency on CopyHeaders I tried this on WTF and its an error. CMake Error in Source/WTF/wtf/CMakeLists.txt: Target "WTF" INTERFACE_INCLUDE_DIRECTORIES property contains path: "C:/webkit/WebKitBuild/Release/WTF/Headers" which is prefixed in the build directory.

Comment on attachment 370453 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370453&action=review >>> Source/bmalloc/CMakeLists.txt:159 >>> +target_include_directories(bmalloc_PostBuild INTERFACE ${bmalloc_FRAMEWORK_HEADERS_DIR}) >> >> I think it would be cleaner to set ${bmalloc_FRAMEWORK_HEADERS_DIR} as INTERFACE include directory on bmalloc target. It will be automatically forwarded to bmalloc_PostBuild alongside with other transitive properties, and it will be clear that the only roles of WebKit::bmalloc target is its dependency on CopyHeaders > > I tried this on WTF and its an error. > > CMake Error in Source/WTF/wtf/CMakeLists.txt: > Target "WTF" INTERFACE_INCLUDE_DIRECTORIES property contains path: > > "C:/webkit/WebKitBuild/Release/WTF/Headers" > > which is prefixed in the build directory. Please you try something like this: target_include_directories(bmalloc INTERFACE "$<BUILD_INTERFACE:${bmalloc_FRAMEWORK_HEADERS_DIR}>") This is the way how we set PUBLIC and PRIVATE include directories in _WEBKIT_TARGET() already

Created attachment 370519 [details] Patch

Comment on attachment 370519 [details] Patch Clearing flags on attachment: 370519 Committed r245723: <https://trac.webkit.org/changeset/245723>

All reviewed patches have been landed. Closing bug.