Page MenuHomePhabricator

Make ninja smart console builds more pretty
ClosedPublic

Authored by davezarzycki on Jun 21 2020, 4:44 AM.

Details

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.

Diff Detail

Event Timeline

davezarzycki created this revision.Jun 21 2020, 4:44 AM

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

davezarzycki added a comment.EditedJun 21 2020, 1:53 PM

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 added a comment.EditedJun 21 2020, 2:25 PM

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 added a comment.EditedJun 21 2020, 3:42 PM

“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 accepted this revision.Jun 21 2020, 8:49 PM

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?

This revision is now accepted and ready to land.Jun 21 2020, 8:49 PM

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.

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?

This revision was automatically updated to reflect the committed changes.

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

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 added inline comments.
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 added inline comments.Aug 18 2020, 4:19 AM
llvm/cmake/modules/VersionFromVCS.cmake
49

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