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

[sameeran joshi via Phabricator via llvm-commits]

sameeranjoshi marked an inline comment as done.
sameeranjoshi 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)
----------------
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


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

https://reviews.llvm.org/D80377