Bug 130496 – [CMake] General improvement checklist

• Status: NEW
• Component: Tools / Tests

[Ben Boeckel]

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