130496 2014-03-19 21:51:46 -0700 [CMake] General improvement checklist 2014-03-19 21:51:46 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified NEW P2 Normal --- 1 mathstuf webkit-unassigned oldest_to_newest 992385 0 mathstuf 2014-03-19 21:51:46 -0700 I've been looking over the CMake code for WebKit recently and here are some of my thoughts: - replace macro() with function() where possible: * functions introduce a new scope and as such, do not need to unset variables or worry about polluting the parent scope with its internal variable workings; * parameters to functions are real variables (macros expand their arguments in the code then execute it; functions set the arguments in the new scope and execute the code); * the last point allows "foreach (f IN LISTS ARGN)" which is cleaner, faster, and safer than "foreach (f ${ARGN})" since the variable expansion isn't done and then parsed causing double-expansion over values with ';' or spaces; - some configure-time logic which could be pushed off to build time (mainly with something like[1]): * reduces the list of files on which configure relies; * configure now does less work; - the (massive) list of include directories for WebKit and WebKit2 could be trimmed down: * can it just be removed due to DerivedSources/ForwardingHeaders being generated? (probably needs CMake to know about these headers though, possibly even build-time generated, so that dependencies work properly); * INTERFACE properties so that linking, e.g., WebCore will bring in WebCore's include directories automatically (though this would require 2.8.12, so probably not doable without conditionals in the near future); - properties are set individually: * properties may be set in one call (e.g., "set_target_properties(JavaScriptCore PROPERTIES COMPILE_DEFINITIONS BUILDING_JavaScriptCore FOLDER JavaScriptCore)", usually with newlines for readability); * some places use "get_property(all) foreach () list(APPEND all) endforeach () set_property(${all})" where set_property(APPEND) would work better (add_source_dependencies (which is actually declared twice) and add_target_properties) (there is a bug prior to 2.8.12 that if APPENDing an empty string, the property would be removed, so that would need to be guarded against); - compiler flag detection/setting could be better: * I've done this using CheckCXXCompilerFlag personally rather than just assuming things; * checking the compiler version manually (with -dumpversion in Source/cmake/WebKitHelpers.cmake) is unnecessary since CMAKE_CXX_COMPILER_VERSION already exists (though upon closer inspection this actually seems to be first available 2.8.8, so maybe that's not possible); * there is a POSITION_INDEPENDENT_CODE target property available in 2.8.9 and up so that -fPIC isn't hard coded (though I don't know if non-GNU-compatible compilers are even cared about, so this may be moot); * setting flags is done per-target when it could be done once with CMAKE_CXX_FLAGS once (maybe this isn't wanted though; I didn't check for everywhere it could be used but isn't). - in the option handling, cmake_dependent_option could be used to express dependencies between options (e.g., VIDEO and VIDEO_TRACK): * I see a comment in GTK's code to hide things from downstream; maybe having mark_as_advanced controlled via an argument to WEBKIT_OPTION_DEFAULT_PORT_VALUE could help there. I can help with most of this, but I'd like to get some feedback before getting too far into it. I can also bring it to the list if that's a better place (filing here so that it acts like a tracking bug and tasks can block it). See also Bug #130493. FWIW, I have also been working on the Ninja generator recently in CMake itself. If all goes well and the patches get merged, the build.ninja file for WebKit should be *much* smaller (~200M -> ~8M for the GTK+ port here) in 3.1 (3.0 was too far into its rc cycle to get the relevant patches merged). [1]See the sprokit_configure_file function here: https://github.com/Kitware/sprokit/blob/master/conf/sprokit-macro-configure.cmake [2]See https://github.com/Kitware/sprokit/blob/master/cmake/snippets/flags.cmake and https://github.com/Kitware/sprokit/blob/master/cmake/snippets/flags-gnu.cmake