Page MenuHomePhabricator

[cmake] Add custom command to touch archives so ninja won't rebuild them.
ClosedPublic

Authored by hintonda on May 20 2019, 9:22 PM.

Details

Summary

clang and ninja use high-resolutions timestamps, but libtool
doesn't, so the archive will often get an older timestamp than the
last object that was added or updated. To fix this, we add a custom
command to touch archive after it's been built so that ninja won't
rebuild it unnecessarily the next time it's run.

Diff Detail

Repository
rL LLVM

Event Timeline

hintonda created this revision.May 20 2019, 9:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2019, 9:22 PM
Herald added a subscriber: mgorny. · View Herald Transcript
beanz added a comment.May 20 2019, 9:50 PM

@thakis Didn't we patch Ninja for this?

@thakis Didn't we patch Ninja for this?

Here's what I'm running on my Mac:

local:/Users/dhinton/projects/llvm_project/monorepo/llvm-project $ ninja --version
1.9.0
local:/Users/dhinton/projects/llvm_project/monorepo/llvm-project $ cmake --version
cmake version 3.14.0

@thakis Didn't we patch Ninja for this?

Here's what I'm running on my Mac:

local:/Users/dhinton/projects/llvm_project/monorepo/llvm-project $ ninja --version
1.9.0
local:/Users/dhinton/projects/llvm_project/monorepo/llvm-project $ cmake --version
cmake version 3.14.0

btw, here's some samples of what I get when I don't touch them:

local:/Users/dhinton/projects/llvm_project/monorepo/build/Debug $ /usr/local/opt/coreutils/libexec/gnubin/ls --full-time lib/lib*.a
-rw-r--r--  1 dhinton staff     17232 2019-05-20 18:03:27.000000000 -0700 libDynamicLibraryLib.a
-rw-r--r--  1 dhinton staff   2822800 2019-05-20 18:01:34.000000000 -0700 libLLVMBinaryFormat.a
-rw-r--r--  1 dhinton staff 605424256 2019-05-20 18:05:56.000000000 -0700 libLLVMCodeGen.a
-rw-r--r--  1 dhinton staff 115587352 2019-05-20 18:04:26.000000000 -0700 libLLVMCore.a

and what I get when I do:

local:/Users/dhinton/projects/llvm_project/monorepo/build/Debug $ /usr/local/opt/coreutils/libexec/gnubin/ls --full-time lib/lib*.a
-rw-r--r-- 1 dhinton staff     17232 2019-05-20 20:57:50.000000000 -0700 lib/libDynamicLibraryLib.a
-rw-r--r-- 1 dhinton staff   2822800 2019-05-20 20:57:50.002389000 -0700 lib/libLLVMBinaryFormat.a
-rw-r--r-- 1 dhinton staff 605424256 2019-05-20 20:58:06.902285000 -0700 lib/libLLVMCodeGen.a
-rw-r--r-- 1 dhinton staff 115587352 2019-05-20 20:58:04.538669000 -0700 lib/libLLVMCore.a

Note that DynamicLibrary doesn't use llvm_add_library...

I have a long story about this issue... Ask me about it sometime :).

We should get @thakis to weigh in here, but Ninja 1.8.2 works on Darwin because it uses the older filesystem APIs and ignores the high-precision timestamps. If we take this patch we should restrict it more. I suspect that libtool on Linux probably behaves correctly. Darwin's binutils fork is literally decades old.

My guess at restricting the patch would be only on Darwin (which is the only place we use libtool), only if the Ninja version > 1.8.2, and Darwin version > 15.6.0 (I think 16.0.0 was when APFS went in which causes this issue).

I have a long story about this issue... Ask me about it sometime :).

We should get @thakis to weigh in here, but Ninja 1.8.2 works on Darwin because it uses the older filesystem APIs and ignores the high-precision timestamps. If we take this patch we should restrict it more. I suspect that libtool on Linux probably behaves correctly. Darwin's binutils fork is literally decades old.

My guess at restricting the patch would be only on Darwin (which is the only place we use libtool), only if the Ninja version > 1.8.2, and Darwin version > 15.6.0 (I think 16.0.0 was when APFS went in which causes this issue).

That sound right. I just did a quick check on a linux box and it didn't have the problem.

hintonda updated this revision to Diff 200517.May 21 2019, 8:16 AM
  • Restrict to Darwin > 15.6.0. Ninja version not available, so will add code to get that and hide all of this under a new variable in the next version of this patch.
hintonda updated this revision to Diff 200521.May 21 2019, 8:53 AM
  • Get Ninja version and add LLVM_TOUCH_STATIC_LIBRARIES.
hintonda updated this revision to Diff 200527.May 21 2019, 9:24 AM
  • Remove unneeded else branch.
beanz added a comment.May 21 2019, 9:36 AM

It is silly that CMake doesn't expose the ninja version, but it is easy enough to get it. It should be something like:

if(CMAKE_GENERATOR STREQUAL "Ninja")
  execute_process(COMMAND ${RunCMake_MAKE_PROGRAM} --version
      OUTPUT_VARIABLE ninja_version OUTPUT_STRIP_TRAILING_WHITESPACE)
  if(ninja_version VERSION_GREATER 1.8.2)
    set(ninja_greater_1_8_2 On)
  endif()
endif()

I know this is a bunch of seemingly odd boiler plate, but since we're working around tools bugs in non-LLVM tools which will hopefully someday be fixed, I'd like to restrict this fix to only places where we know the bug is present.

hintonda updated this revision to Diff 200531.May 21 2019, 9:43 AM
  • Don't put new variable in the cache.

It is silly that CMake doesn't expose the ninja version, but it is easy enough to get it. It should be something like:

if(CMAKE_GENERATOR STREQUAL "Ninja")
  execute_process(COMMAND ${RunCMake_MAKE_PROGRAM} --version
      OUTPUT_VARIABLE ninja_version OUTPUT_STRIP_TRAILING_WHITESPACE)
  if(ninja_version VERSION_GREATER 1.8.2)
    set(ninja_greater_1_8_2 On)
  endif()
endif()

I know this is a bunch of seemingly odd boiler plate, but since we're working around tools bugs in non-LLVM tools which will hopefully someday be fixed, I'd like to restrict this fix to only places where we know the bug is present.

I already added it to llvm/cmake/config-ix.cmake. Please let me know if it looks okay.

hintonda updated this revision to Diff 200532.May 21 2019, 9:52 AM
  • Remove tab.
beanz accepted this revision.May 21 2019, 10:00 AM

One small change needed, otherwise LGTM.

llvm/cmake/config-ix.cmake
569 ↗(On Diff #200532)

Instead of checking CMAKE_SYSTEM_NAME you should probably just check CMAKE_HOST_APPLE. Not that people really are targeting Darwin from other hosts, but CMAKE_SYSTEM_NAME is actually the target system not the host.

This revision is now accepted and ready to land.May 21 2019, 10:00 AM
hintonda updated this revision to Diff 200540.May 21 2019, 10:46 AM
  • Use CMAKE_HOST_APPLE when checking host system.
This revision was automatically updated to reflect the committed changes.
ABataev added inline comments.
llvm/trunk/cmake/config-ix.cmake
558

This line breaks cmake, seems to me must be CMakeNinjaFindMake