[DO NOT MERGE] Check if #23056 would have caught alma8+ninja issues with #21346

.github/workflows/root-ci-config/build_root.py

@@ -24,6 +24,7 @@
 import time
 
 import build_utils
+import null_build_check
 from build_utils import (
     calc_options_hash,
     die,
@@ -174,6 +175,10 @@ def main():
 
     build(options, args.buildtype)
 
+    # Done before anything else touches the build tree, and reported only at the
+    # very end so that a spurious rebuild does not cost us the test results.
+    null_build_ok = check_for_spurious_rebuilds(args.buildtype)
+
     # Build artifacts should only be uploaded for full builds, and only for
     # "official" branches (master, v?-??-??-patches), i.e. not for pull_request
     # We also want to upload any successful build, even if it fails testing
@@ -201,6 +206,12 @@ def main():
     if testing and ctest_returncode != 0:
         handle_test_failure(ctest_returncode)
 
+    if not null_build_ok:
+        die(
+            msg="Building an already built ROOT rebuilt files it should not have; "
+            'see the "Check for spurious rebuilds" step above'
+        )
+
     print_trace()
 
 def handle_test_failure(ctest_returncode):
@@ -443,20 +454,49 @@ def dump_requested_config(options):
     print(f"\nBUILD OPTIONS: {options}")
 
 
-@github_log_group("Build")
-def cmake_build(buildtype):
+def cmake_build_command(buildtype) -> str:
     generator_flags = "-- '-verbosity:minimal' '-consoleloggerparameters:summary'" if WINDOWS else ""
     parallel_jobs = "4" if WINDOWS else str(os.cpu_count())
 
     builddir = os.path.join(WORKDIR, "build")
-    result = subprocess_with_log(f"""
+
+    return f"""
         cmake --build '{builddir}' --config '{buildtype}' --parallel '{parallel_jobs}' {generator_flags}
-    """)
+    """
+
+
+@github_log_group("Build")
+def cmake_build(buildtype):
+    result = subprocess_with_log(cmake_build_command(buildtype))
 
     if result != 0:
         die(result, "Failed to build")
 
 
+@github_log_group("Check for spurious rebuilds")
+def check_for_spurious_rebuilds(buildtype) -> bool:
+    """Build a second time and check that there was nothing left to do.
+
+    Returns whether the build tree was already up to date, i.e. whether the
+    second build wrote nothing but the files listed in null_build_check.py.
+    """
+    builddir = os.path.join(WORKDIR, "build")
+
+    def rebuild() -> int:
+        return subprocess_with_capture(cmake_build_command(buildtype)).returncode
+
+    try:
+        touched = null_build_check.check_null_build(builddir, rebuild)
+    except RuntimeError as err:
+        # Not a spurious rebuild, but something we still want to know about.
+        build_utils.print_warning(f"Could not check for spurious rebuilds: {err}")
+        return True
+
+    null_build_check.report(builddir, touched)
+
+    return not touched
+
+
 def build(options, buildtype):
     if not os.path.isdir(os.path.join(WORKDIR, "build")):
         builddir = os.path.join(WORKDIR, "build")

.github/workflows/root-ci-config/buildconfig/alma8.txt

@@ -1,8 +1,10 @@
+CMAKE_GENERATOR=Ninja
 builtin_civetweb=ON
 builtin_gtest=ON
 builtin_nlohmannjson=ON
 builtin_tbb=ON
 builtin_vdt=ON
 curl=OFF
+fortran=OFF
 pythia8=ON
 use_gsl_cblas=ON

.github/workflows/root-ci-config/null_build_check.py

@@ -0,0 +1,196 @@
+#!/usr/bin/env python3
+
+# pylint: disable=missing-function-docstring,line-too-long
+
+"""Check that building an already built tree does not rebuild anything.
+
+Generator agnostic: rather than parsing build tool output, it fingerprints every
+file in the build tree, builds again, and reports what was written. ROOT's one
+accepted exception is etc/gitinfo.txt, which the gitinfotxt target in the top
+level CMakeLists.txt rewrites on every build by design.
+
+Standalone usage:
+
+    python3 null_build_check.py <builddir> [<extra cmake --build args>...]
+"""
+
+import argparse
+import collections
+import fnmatch
+import os
+import subprocess
+import sys
+
+# Globs matched against the '/'-separated path relative to the build directory.
+# fnmatch's '*' matches '/' too, so unanchored patterns match at any depth.
+ALLOWED_PATTERNS = (
+    "etc/gitinfo.txt",
+    # Build tool logs and dependency databases, not build products.
+    ".ninja_log",
+    ".ninja_deps",
+    ".ninja_lock",
+    ".cmake/api/*",
+    # Refreshed by every make.
+    "*/compiler_depend.*",
+    "*/depend.internal",
+    "*/depend.make",
+    "CMakeFiles/progress.marks",
+    # MSBuild file trackers and up-to-date markers.
+    "*.tlog",
+    "*.tlog/*",
+    "*.lastbuildstate",
+    "*.unsuccessfulbuild",
+    "*.CopyComplete",
+    # MSBuild writes the link recipe of every project on every build, whether
+    # or not the project is relinked.
+    "*.recipe",
+    # The ZERO_CHECK project of the Visual Studio generator rewrites the CMake
+    # generation stamps on every build to record that CMake need not re-run. A
+    # CMake run that really did regenerate would still be caught, since it
+    # rewrites the project files as well.
+    "CMakeFiles/generate.stamp",
+    "*/CMakeFiles/generate.stamp",
+    # ctest leftovers, in case the tree has been tested before.
+    "Testing/*",
+)
+
+# ExternalProject keeps git clones of externals in the build tree.
+PRUNED_DIRS = (".git",)
+
+MAX_REPORTED = 100
+
+
+def snapshot(builddir: str) -> dict:
+    fingerprints = {}
+
+    for dirpath, dirnames, filenames in os.walk(builddir):
+        dirnames[:] = [name for name in dirnames if name not in PRUNED_DIRS]
+
+        for filename in filenames:
+            path = os.path.join(dirpath, filename)
+            try:
+                stat = os.lstat(path)
+            except OSError:
+                continue  # vanished under us, or a dangling symlink
+            key = os.path.relpath(path, builddir).replace(os.sep, "/")
+            fingerprints[key] = (stat.st_mtime_ns, stat.st_size)
+
+    return fingerprints
+
+
+def is_allowed(path: str) -> bool:
+    return any(fnmatch.fnmatch(path, pattern) for pattern in ALLOWED_PATTERNS)
+
+
+def compare(before: dict, after: dict) -> list:
+    touched = []
+
+    for path, fingerprint in after.items():
+        if path not in before:
+            touched.append((path, "created"))
+        elif before[path] != fingerprint:
+            touched.append((path, "modified"))
+
+    for path in before:
+        if path not in after:
+            touched.append((path, "deleted"))
+
+    return sorted(entry for entry in touched if not is_allowed(entry[0]))
+
+
+def ninja_explain(builddir: str) -> str:
+    if not os.path.exists(os.path.join(builddir, "build.ninja")):
+        return ""
+
+    try:
+        result = subprocess.run(
+            ["ninja", "-C", builddir, "-n", "-d", "explain"],
+            capture_output=True,
+            text=True,
+            errors="replace",
+            check=False,
+        )
+    except OSError:
+        return ""
+
+    return result.stderr
+
+
+def check_null_build(builddir: str, rebuild) -> list:
+    """`rebuild` runs `cmake --build builddir` and returns its exit code."""
+
+    print(f"Recording the state of {builddir}")
+    before = snapshot(builddir)
+    print(f"{len(before)} files")
+
+    returncode = rebuild()
+    if returncode != 0:
+        raise RuntimeError(f"rebuilding an already built tree failed with exit code {returncode}")
+
+    return compare(before, snapshot(builddir))
+
+
+def summarize(touched: list) -> list:
+    """Count the reported paths per file type, most numerous first."""
+
+    counts = collections.Counter()
+
+    for path, _ in touched:
+        name = path.rsplit("/", 1)[-1]
+        stem, dot, extension = name.rpartition(".")
+        counts["*." + extension if stem and dot else name] += 1
+
+    return counts.most_common()
+
+
+def report(builddir: str, touched: list) -> None:
+    if not touched:
+        print("No spurious rebuilds: building again left the build tree untouched.")
+        return
+
+    print(f"{len(touched)} file(s) were written by a build that had nothing to do:")
+
+    for path, what in touched[:MAX_REPORTED]:
+        print(f"  {what:<8}  {path}")
+
+    if len(touched) > MAX_REPORTED:
+        print(f"  ... and {len(touched) - MAX_REPORTED} more")
+        # The listing is truncated and sorted by path, so a category that only
+        # shows up late would go unnoticed without this.
+        print("\nBy file type:")
+        for kind, count in summarize(touched):
+            print(f"  {count:>6}  {kind}")
+
+    explanation = ninja_explain(builddir)
+    if explanation:
+        print("\nWhy ninja thinks there is work left to do:")
+        print(explanation)
+
+    print("""
+Usual causes are a custom command that does not declare its OUTPUT or BYPRODUCTS,
+an output that ends up newer than what it is compared against, or a generated
+file embedding a timestamp. If a file really has to be rewritten on every build,
+add it to ALLOWED_PATTERNS above.""")
+
+
+def main() -> int:
+    parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter)
+    parser.add_argument("builddir", help="a ROOT build directory that is fully built")
+    parser.add_argument("build_args", nargs="*", help="extra arguments for `cmake --build`, e.g. --config Release")
+    args = parser.parse_args()
+
+    builddir = os.path.abspath(args.builddir)
+    command = ["cmake", "--build", builddir, "--parallel", str(os.cpu_count())] + args.build_args
+
+    def rebuild() -> int:
+        print("+ " + " ".join(command))
+        return subprocess.run(command, check=False).returncode
+
+    touched = check_null_build(builddir, rebuild)
+    report(builddir, touched)
+
+    return 1 if touched else 0
+
+
+if __name__ == "__main__":
+    sys.exit(main())

cmake/modules/RootMacros.cmake

@@ -685,10 +685,10 @@ function(ROOT_GENERATE_DICTIONARY dictionary)
   endforeach()
 
   #---build the implicit dependencies arguments
-  # NOTE: only the Makefile generator respects this!
-  foreach(_dep ${_linkdef} ${_list_of_header_dependencies})
-    list(APPEND _implicitdeps CXX ${_dep})
-  endforeach()
+  # NOTE: DEPFILE is used instead of IMPLICIT_DEPENDS because IMPLICIT_DEPENDS
+  # only works with Unix Makefiles and has issues with cross-directory dependencies.
+  # DEPFILE works with all generators (Ninja, Unix Makefiles, etc.)
+  set(depfile_path ${CMAKE_CURRENT_BINARY_DIR}/${dictionary}.depfile)
 
   if(ARG_MODULE)
     set(MODULE_LIB_DEPENDENCY ${ARG_DEPENDENCIES})
@@ -741,7 +741,8 @@ function(ROOT_GENERATE_DICTIONARY dictionary)
                        # make the dictionary generation command depend on the C++ standard, ensuring that the
                        # dictionaries will be rebuilt if the C++ standard is changed in an incremental build.
                        -DR__DUMMY_CXX_STANDARD_${CMAKE_CXX_STANDARD}
-    IMPLICIT_DEPENDS ${_implicitdeps}
+                       -MF ${depfile_path}
+    DEPFILE ${depfile_path}
     DEPENDS ${_list_of_header_dependencies} ${_linkdef} ${ROOTCLINGDEP}
             ${pcm_dependencies}
             ${MODULE_LIB_DEPENDENCY} ${ARG_EXTRA_DEPENDENCIES}
@@ -1371,10 +1372,20 @@ function(ROOT_STANDARD_LIBRARY_PACKAGE libname)
     set(NO_CXXMODULE_FLAG "NO_CXXMODULE")
   endif()
 
+  set(dummy_source)
   if(ARG_NO_SOURCES)
     # Workaround bug in CMake by adding a dummy source file if all sources are generated, since
     # in that case the initial call to add_library() may not list any sources and CMake complains.
-    add_custom_command(OUTPUT dummy.cxx COMMAND ${CMAKE_COMMAND} -E touch dummy.cxx)
+    #
+    # The file is written here at configure time rather than by a custom command on purpose: all
+    # the packages of one directory share it, and listing one custom command output in several
+    # independent targets that build in parallel is not supported. With MSBuild the touch ran
+    # again on every build, so the 15 STL dictionaries of core/clingutils kept recompiling and
+    # relinking, while Ninja and Make happened to get away with it.
+    set(dummy_source ${CMAKE_CURRENT_BINARY_DIR}/dummy.cxx)
+    if(NOT EXISTS ${dummy_source})
+      file(WRITE ${dummy_source} "")
+    endif()
   endif()
 
   if(runtime_cxxmodules)
@@ -1390,16 +1401,14 @@ function(ROOT_STANDARD_LIBRARY_PACKAGE libname)
   endif()
 
   if (ARG_OBJECT_LIBRARY)
-    ROOT_OBJECT_LIBRARY(${libname}Objs ${ARG_SOURCES}
-                        $<$<BOOL:${ARG_NO_SOURCES}>:dummy.cxx>)
+    ROOT_OBJECT_LIBRARY(${libname}Objs ${ARG_SOURCES} ${dummy_source})
     ROOT_LINKER_LIBRARY(${libname} $<TARGET_OBJECTS:${libname}Objs>
                         LIBRARIES ${ARG_LIBRARIES}
                         DEPENDENCIES ${ARG_DEPENDENCIES}
                         BUILTINS ${ARG_BUILTINS}
                        )
   else(ARG_OBJECT_LIBRARY)
-    ROOT_LINKER_LIBRARY(${libname} ${ARG_SOURCES}
-                        $<$<BOOL:${ARG_NO_SOURCES}>:dummy.cxx>
+    ROOT_LINKER_LIBRARY(${libname} ${ARG_SOURCES} ${dummy_source}
                         LIBRARIES ${ARG_LIBRARIES}
                         DEPENDENCIES ${ARG_DEPENDENCIES}
                         BUILTINS ${ARG_BUILTINS}