Page MenuHomePhabricator

Add install targets for gtest
AcceptedPublic

Authored by tstellar on Nov 12 2022, 3:30 AM.

Details

Reviewers
mgorny
kwk
Summary

Stand-alone builds need an installed version of gtest in order to run
the unittests.

Diff Detail

Unit TestsFailed

TimeTest
470 msx64 debian > DataFlowSanitizer-x86_64.DataFlowSanitizer-x86_64::conditional_callbacks.c
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fsanitize=dataflow -m64 -fno-sanitize=dataflow -O2 -fPIE -DCALLBACKS -c /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/dfsan/conditional_callbacks.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/dfsan/X86_64Config/Output/conditional_callbacks.c.tmp-callbacks.o
80 msx64 windows > MLIR.Dialect/SparseTensor::codegen_buffer_initialization.mlir
Script: -- : 'RUN: at line 1'; c:\ws\w3\llvm-project\premerge-checks\build\bin\mlir-opt.exe C:\ws\w3\llvm-project\premerge-checks\mlir\test\Dialect\SparseTensor\codegen_buffer_initialization.mlir --sparse-tensor-codegen=enable-buffer-initialization=true --canonicalize --cse | c:\ws\w3\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w3\llvm-project\premerge-checks\mlir\test\Dialect\SparseTensor\codegen_buffer_initialization.mlir

Event Timeline

tstellar created this revision.Nov 12 2022, 3:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2022, 3:30 AM
tstellar requested review of this revision.Nov 12 2022, 3:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2022, 3:30 AM
kwk added a subscriber: kwk.Nov 14 2022, 6:23 AM

I'll try to test this today. I suppose the main idea would be to enable LLVM_INSTALL_GTEST, then stop unpacking third-party directory when building other packages, correct? Perhaps it would make sense to update the checks like the one in clang while doing that:

if( CLANG_INCLUDE_TESTS )
  if(EXISTS ${LLVM_THIRD_PARTY_DIR}/unittest/googletest/include/gtest/gtest.h)
    add_subdirectory(unittests)
    ...

Plus there's the matter of LLVMTestingSupport library.

I'll try to test this today. I suppose the main idea would be to enable LLVM_INSTALL_GTEST, then stop unpacking third-party directory when building other packages, correct? Perhaps it would make sense to update the checks like the one in clang while doing that:

if( CLANG_INCLUDE_TESTS )
  if(EXISTS ${LLVM_THIRD_PARTY_DIR}/unittest/googletest/include/gtest/gtest.h)
    add_subdirectory(unittests)
    ...

I submitted the clang patch here: D138472

mgorny added inline comments.Nov 22 2022, 5:15 AM
third-party/unittest/CMakeLists.txt
81

Any reason you think this should be installed separately rather than included in the usual LLVM export list? I think the latter would make it more consistent to use in in-tree vs standalone builds (as you wouldn't have to find_package it).

tstellar added inline comments.Nov 22 2022, 8:43 AM
third-party/unittest/CMakeLists.txt
81

The problem with having it in the main export list is that it then it would have to be present on disk for users to be able to load LLVMConfig.cmake.

81

I was just trying to make the patch more self-contained, so it wouldn't risk breaking other build configurations, but I can add it into the main export list if you think that's better.

mgorny added inline comments.Nov 22 2022, 9:08 AM
third-party/unittest/CMakeLists.txt
81

I suppose you're thinking of packaging it separate from llvm-devel then? If not, then I think it'd be better to keep it simple and include it in main exports. After all, most users simply won't enable the option to install it, so shouldn't be affected.

kwk accepted this revision.Wed, Jan 25, 8:50 AM

Works for me.

This revision is now accepted and ready to land.Wed, Jan 25, 8:50 AM