[flang-commits] [PATCH] D80377: [flang] Google test infrastructure support for unittests

[Tim Keith via Phabricator via flang-commits]

tskeith added inline comments.


================
Comment at: flang/CMakeLists.txt:128
+  set(UNITTEST_DIR ${LLVM_BUILD_MAIN_SRC_DIR}/utils/unittest)
+  if(EXISTS ${UNITTEST_DIR}/googletest/include/gtest/gtest.h)
+    if (TARGET gtest)
----------------
sameeranjoshi wrote:
> Meinersbur wrote:
> > tskeith wrote:
> > > sameeranjoshi wrote:
> > > > tskeith wrote:
> > > > > Why do you have to check for `gtest.h`? Won't it always be there?
> > > > Not always because
> > > > 
> > > > > 
> > > > > The out-of-tree build can be configured against the CMAKE_INSTALL_PREFIX directory of llvm, rather than the build directory. 
> > > > > LLVM does not install gtest when installing itself, meaning that gtest might not be available.
> > > > > 
> > > > 
> > > > Also other subprojects as well follow something similar
> > > > [1]https://github.com/llvm/llvm-project/blob/8e058feae0b0d07cd86257f0aa3154acfa887fe0/clang/CMakeLists.txt#L188
> > > > [2]https://github.com/llvm/llvm-project/blob/82aac878beb48cd326b4684918b7ff2375fae439/polly/CMakeLists.txt#L29
> > > > [3]https://github.com/llvm/llvm-project/blob/82aac878beb48cd326b4684918b7ff2375fae439/lld/CMakeLists.txt#L109
> > > > 
> > > > 
> > > If the user requests unit tests and we can't run them it should be an error, not silently ignored.
> > If the user is running `make FlangUnitTests`, yes. The build target may not exist in this case. But failing on `make check-flang` would mean the build cannot be verified at all if configuring against a pre-installed LLVM. If not supported, I would question the point of a flang standalone build. 
> > 
> > Note that it is never possible to run all tests all the time since some tests have requirements, such as running on Windows. A warning about skipped tests might be useful though.
> Agreed that a warning is useful.
> 
> What if we use `CMake to download GoogleTest as part of the build's configure step[1]  specifically for standalone builds`? It definitely would increase configuration time, but we are assured the unit tests they run perfectly well.
> 
> [1]https://github.com/google/googletest/blob/master/googletest/README.md#incorporating-into-an-existing-cmake-project
I think it would make more sense to install GoogleTest as part of the LLVM install, as it currently done with testing tools like llvm-lit and FileCheck. Then this should work the same whether built against a build or install of LLVM. If you decide to do this it should be a separate change.

In the meantime, a meaningful warning is fine with me.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80377/new/

https://reviews.llvm.org/D80377