Make ninja smart console builds more pretty

[davezarzycki]

Summary

CMake's find_package outputs to the console on success, which confuses the smart console mode of the ninja build system. Let's quiet the success message and manually warn instead.

[mehdi_amini]

Just curious: what is the practical effect of the change? (I don't know what is "ninja smart console")

[davezarzycki]

Naive build output is one line per file (plus output).

Ninja’s smart console mode overwrites the current line with whatever is building at the moment and once everything builds successfully there is only one line of output. That being said, if something forcibly writes to the console, then there are hiccups in the output.

[mehdi_amini]

OK!
So I renamed the find_package(Git) into find_package(Git2) to force an error:

...
-- LLVM host triple: x86_64-unknown-linux-gnu
-- LLVM default target triple: x86_64-unknown-linux-gnu
-- Building with -fPIC
-- Constructing LLVMBuild project information
CMake Warning at cmake/modules/VersionFromVCS.cmake:7 (find_package):
  By not providing "FindGit2.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "Git2", but
  CMake did not find one.

  Could not find a package configuration file provided by "Git2" with any of
  the following names:

    Git2Config.cmake
    git2-config.cmake

  Add the installation prefix of "Git2" to CMAKE_PREFIX_PATH or set
  "Git2_DIR" to a directory containing one of the above files.  If "Git2"
  provides a separate development package or SDK, be sure it has been
  installed.
Call Stack (most recent call first):
  CMakeLists.txt:851 (get_source_info)


-- Targeting X86
-- Targeting AMDGPU
-- Targeting NVPTX
-- Registering Bye as a pass plugin (static build: OFF)
-- Failed to find LLVM FileCheck
-- Version: 0.0.0
-- Performing Test HAVE_THREAD_SAFETY_ATTRIBUTES -- failed to compile
-- Performing Test HAVE_GNU_POSIX_REGEX -- failed to compile
-- Performing Test HAVE_POSIX_REGEX -- success
-- Performing Test HAVE_STEADY_CLOCK -- success
-- Configuring done
-- Generating done
-- Build files have been written to: llvm-project/build
[211/300] Linking CXX executable bin/Kaleido

The build has been progressing till the end, overwriting the progress:

[300/300] Generating ../../bin/llvm-readelf

How can I observe the effect of your change on ninja?

[davezarzycki]

“N/300” implies that you’re not building all of llvm, clang, etc otherwise thousands of files would build.

I regularly build llvm plus clang, lld, libcxx, libcxxabi, and compiler-rt. in such a build, find_package(Git) is called three times. And therefore the ninja output is disrupted three times.

Have you tried a clean build?

[mehdi_amini]

OK I see now, the description wasn't clear to me earlier. So it is about the fact that find_package failure is multiple-lines long, and it is called separately from LLVM, clang, lld, and lldb. Instead we will still display 4 times the warning but it'll be one line instead, right?

Seems like we could also call this once only from the LLVM CMakeLists.txt and make the variables available to the subprojects?

[phosek]

In D82276#2105872, @mehdi_amini wrote:

OK I see now, the description wasn't clear to me earlier. So it is about the fact that find_package failure is multiple-lines long, and it is called separately from LLVM, clang, lld, and lldb. Instead we will still display 4 times the warning but it'll be one line instead, right?

Seems like we could also call this once only from the LLVM CMakeLists.txt and make the variables available to the subprojects?

I think that would be better. The current setup predates monorepo where LLVM, clang, lld and lldb were in separate repositories, but now when they're in the same repository, there's no reason to invoke get_source_info multiple times.

[davezarzycki]

Unless I'm missing something, consolidating this logic into LLVM's CMakeLists.txt will break stand alone builds of sub-projects and external projects that depend on this logic.

Was there a policy change? Are stand alone builds no longer supported? Are external projects no longer allowed to use LLVM's GenerateVersionFromVCS.cmake?

[mehdi_amini]

Couldn't we "cache" the result of the check in the function get_source_info?

[phosek]

In D82276#2106260, @davezarzycki wrote:

Unless I'm missing something, consolidating this logic into LLVM's CMakeLists.txt will break stand alone builds of sub-projects and external projects that depend on this logic.

Was there a policy change? Are stand alone builds no longer supported? Are external projects no longer allowed to use LLVM's GenerateVersionFromVCS.cmake?

There wasn't, but we only need to call get_source_info from clang, lld or lldb when they're being built as standalone. When being built as part of LLVM, we can just reuse the LLVM revision and repository.

[aaronpuchert]

llvm/cmake/modules/VersionFromVCS.cmake

49

Why a WARNING and not just a STATUS? When git can't be found the probability is high that this isn't a repo checkout but just a build from tarballs.

Alternatively we could try to detect if a .git directory is there and not warn if it isn't.

[davezarzycki]

llvm/cmake/modules/VersionFromVCS.cmake

49

WARNING was used because it preserved the existing behavior when git cannot be found. STATUS seems fine too.

llvm/cmake/modules/VersionFromVCS.cmake

 # Adds version control information to the variable VERS. For
 # determining the Version Control System used (if any) it inspects the
 # existence of certain subdirectories under SOURCE_DIR (if provided as an
 # extra argument, otherwise uses CMAKE_CURRENT_SOURCE_DIR).
 
 function(get_source_info path revision repository)
-  find_package(Git)
+  find_package(Git QUIET)
   if(GIT_FOUND)
     execute_process(COMMAND ${GIT_EXECUTABLE} rev-parse --git-dir
       WORKING_DIRECTORY ${path}
       RESULT_VARIABLE git_result
       OUTPUT_VARIABLE git_output
       ERROR_QUIET)
     if(git_result EQUAL 0)
       string(STRIP "${git_output}" git_output)
if(git_result EQUAL 0)
         ERROR_QUIET)
       if(git_result EQUAL 0)
         string(STRIP "${git_output}" git_output)
         set(${repository} ${git_output} PARENT_SCOPE)
       else()
         set(${repository} ${path} PARENT_SCOPE)
       endif()
     endif()
+  else()
+    message(WARNING "Git not found. Version cannot be determined.")
   endif()
 endfunction()