Adds google test infrastructure support for in tree and out-of-tree. Updates README. Adds new target `check-flang-unit`
Details
Diff Detail
Event Timeline
When I try to build I get this error:
ld: library not found for -lgtest_main
Please update the README to indicate how to do the correct setup. A link to elsewhere in LLVM is fine.
flang/README.md | ||
---|---|---|
168 ↗ | (On Diff #265675) | How do you run unit tests for out-of-tree builds? |
From a cursory look #include <gtest/gtest.h>should have been #include "gtest/gtest.h". As I had gtest package installed.
I tried twice building the compiler in-tree on 2 different m/c. I couldn't reproduce the issue, here's how my cmake looks like:
cmake -G "Ninja" -DCMAKE_INSTALL_PREFIX=../install/ -DCMAKE_BUILD_TYPE="Release" -DLLVM_TARGETS_TO_BUILD=host -DLLVM_ENABLE_PROJECTS="mlir;flang" -DCMAKE_CXX_STANDARD=17 -DFLANG_INCLUDE_TESTS=ON ../llvm-project/llvm/
Where is it finding the gtest_main library? Is it built as part of llvm or coming from outside?
Googletest is built with LLVM, source should be in llvm-project/llvm/utils/unittest/googletest
Googletest is built with LLVM, source should be in llvm-project/llvm/utils/unittest/googletest
Also you should try considering below commit in fir-dev.
https://github.com/flang-compiler/f18-llvm-project/pull/91
This would add the check-flang-unit target, without the FIR dependency, so the other pending post merge task
https://github.com/flang-compiler/f18/issues/995
could not have a blocker.
And the out-of-tree building would still fail as there are issues in building flang with out of tree with FLANG_INCLUDE_TESTS=ON, that's the reason I didn't mention in README
Where is it finding the gtest_main library? Is it built as part of llvm or coming from outside?
Googletest is built with LLVM, source should be in llvm-project/llvm/utils/unittest/googletest
Where is gtest_main found in your build?
Where is gtest_main found in your build?
It's in build/lib/libgtest_main.a
Thanks, I found the problem: I had LLVM_INCLUDE_TESTS set to Off and apparently it must be On for gtest to get built.
So the cmake files should either force it to be on when FLANG_INCLUDE_TESTS is on, or report a fatal error when they are set inconsistently.
Thanks @sameeranjoshi for working on this.
check-flang-unit works for me with ninja on Ubuntu Linux on AArch64 in-tree build.
Please wait for @tskeith to approve.
flang/CMakeLists.txt | ||
---|---|---|
132 ↗ | (On Diff #265675) |
|
flang/CMakeLists.txt | ||
---|---|---|
132 ↗ | (On Diff #265675) | I don't think this is right. LLVM_INCLUDE_TESTS is On by default according to llvm/docs/CMake.rst. LLVM so we should be able to keep the original code. This is what the other LLVM sub-projects seem to do, i.e. give you a local toggle for the unit tests of the sub-project, but default to the global setting for unit tests. Note there is LLVM_BUILD_TESTS, which defaults to OFF, that excludes the tests from the all target. This means the unittests will be built only on make check-all rather than on make. |
I think the code to handle the issue that @tskeith points out in clang standalone builds is here https://github.com/llvm/llvm-project/blob/master/clang/CMakeLists.txt#L187. The key is to add a dependency on the utils/unittest directory.
There is also a GTest format that should be set in the lit.cfg https://github.com/llvm/llvm-project/blob/master/clang/test/Unit/lit.cfg.py#L24.
flang/test/Unit/lit.cfg.py | ||
---|---|---|
30 ↗ | (On Diff #265675) | Why is LLVM_SRC_ROOT needed for the tests? |
33 ↗ | (On Diff #265675) | Other sub-projects set this up in top-level CMakeLists using FindPythonInterp. Why do we set it here? |
flang/test/Unit/lit.cfg.py | ||
---|---|---|
33 ↗ | (On Diff #265675) | Sameeran was pulling this glue code from the lld subproject, where these config settings are made. They can probably be deleted here. I do not appear to be used, but I haven't tested it. |
flang/CMakeLists.txt | ||
---|---|---|
132 ↗ | (On Diff #265675) | Agreed. This should not be changed. |
All the changes were made for handling out-of-tree errors, no changes in in-tree handling code.
- Some changes were ported from fir-dev to resolve out-of-tree errors and some API changes which were bugs and merged in fir-dev .
- A sample gtest is added.
- Changes in Cmake to support out-of-tree builds of unittests
- README updated for out-of-tree setup
note: check-flang-unit won't work for out-of-tree till D81002 is merged.
It seems to me there is something wrong with this patch. The paths are relative to flang/ rather that the root of the repo so the patch doesn't apply successfully. Or maybe there is something I'm missing about how phabricator works?
The root of the repo is llvm-project, of which flang is a subdirectory. The patch seems incorrect to me too on that basis.
Sorry I made a mistake. I was working out-of tree and that caused me to think in usual way when updating diff.
Please check this and thanks for pointing.
flang/CMakeLists.txt | ||
---|---|---|
128 ↗ | (On Diff #267924) | Why do you have to check for gtest.h? Won't it always be there? |
129 ↗ | (On Diff #267924) | Same question. Why do you have to check this? |
flang/include/flang/Optimizer/Support/InternalNames.h | ||
68 ↗ | (On Diff #267924) | This doesn't look properly formatted. Is it part of your change? |
flang/runtime/io-error.cpp | ||
10 ↗ | (On Diff #267924) | Is this supposed to be part of your change? |
flang/runtime/io-error.cpp | ||
---|---|---|
10 ↗ | (On Diff #267924) | Yes, out-of-tree builds fail with this include. |
flang/CMakeLists.txt | ||
---|---|---|
128 ↗ | (On Diff #267924) | Not always because
Also other subprojects as well follow something similar |
flang/include/flang/Optimizer/Support/InternalNames.h | ||
68 ↗ | (On Diff #267924) | Yes it is. |
flang/test/Unit/lit.cfg.py | ||
33 ↗ | (On Diff #265675) | Removed them as they are not used currently. |
flang/CMakeLists.txt | ||
---|---|---|
128 ↗ | (On Diff #267924) | If the user requests unit tests and we can't run them it should be an error, not silently ignored. |
flang/include/flang/Optimizer/Support/InternalNames.h | ||
68 ↗ | (On Diff #267924) | This should be in a separate change. It has nothing to do with google test infrastructure. |
flang/runtime/io-error.cpp | ||
10 ↗ | (On Diff #267924) | This should be in a separate change. It has nothing to do with google test infrastructure. The include of config.h was added for a reason. Does that reason no longer apply or are you breaking something else by changing this? |
flang/runtime/io-error.cpp | ||
---|---|---|
10 ↗ | (On Diff #267924) | I could build and test successfully with removing this file for out of tree and in tree. |
flang/runtime/io-error.cpp | ||
---|---|---|
10 ↗ | (On Diff #267924) | You can answer this question with git log: commit ea5efd1ea87e3d6b86a8e7f940f94dec0a90a004 Author: Isuru Fernando <isuruf@gmail.com> Date: Thu Mar 12 15:28:35 2020 -0500 [flang] Support platforms without strerror_r Original-commit: flang-compiler/f18@0575b54cc79110ae03f73b136e704f6313e2d63f Reviewed-on: https://github.com/flang-compiler/f18/pull/1068 |
flang/runtime/io-error.cpp | ||
---|---|---|
10 ↗ | (On Diff #267924) | Addressed by https://reviews.llvm.org/D81266 |
flang/CMakeLists.txt | ||
---|---|---|
128 ↗ | (On Diff #267924) | 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. |
flang/CMakeLists.txt | ||
---|---|---|
155 ↗ | (On Diff #269340) | I get this. Is it what you're expecting? CMake Error at CMakeLists.txt:164 (message): gtest.h is missing from /utils/unittest/googletest/include/gtest |
flang/README.md | ||
168 ↗ | (On Diff #269340) | Somewhere in here you should explain that you can't run unit tests in an out-of-tree build against an LLVM install. |
flang/CMakeLists.txt | ||
---|---|---|
128 ↗ | (On Diff #267924) | 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. |
flang/CMakeLists.txt | ||
---|---|---|
128 ↗ | (On Diff #267924) | 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. |
Have whatever changes made here been propagated to the fir-dev branch? I don't want to try to decipher which changes are needed, why things fail (if they do), etc.
The status of the fir-dev branch has no bearing on this patch review.
If @sameeranjoshi wants to update fir-dev after pushing this change to master then that is up to him, but that should not in any way block committing the code to master.
flang/CMakeLists.txt | ||
---|---|---|
160 ↗ | (On Diff #270037) | I think it's better to use CMake's find_package(Threads) support here, so that this is more likely work on platforms without pthreads. |
flang/CMakeLists.txt | ||
---|---|---|
132 ↗ | (On Diff #270448) | Is the variable LLVM_INCLUDE_TESTS set in a out-of-tree build? |
flang/CMakeLists.txt | ||
---|---|---|
132 ↗ | (On Diff #270448) | For an out-of-tree build of flang the user is supposed to set FLANG_INCLUDE_TESTS if he wants a local toggle for the unit tests of sub project(flang in this case). Now LLVM_INCLUDE_TESTS depends on how LLVM was built, but by default LLVM_INCLUDE_TESTS is ON. Can read more on this on my older revision[1] |
flang/CMakeLists.txt | ||
---|---|---|
132 ↗ | (On Diff #270448) | LLVM_INCLUDE_TESTS is not set to any value in an out-of-tree build. It's only set if you are doing an in-tree build. |
flang/CMakeLists.txt | ||
---|---|---|
132 ↗ | (On Diff #270448) | This patch seems to be ready for in-tree testing, which is great! We are keen to start adding unit tests - could the discussion about out-of-tree testing be moved to a separate patch? Ta! |
@isuruf I have set FLANG_INCLUDE_TESTS to ON by default for out-of-tree builds.
If there are still any concerns I can fix in other PR.
@awarzynski I have merged changes[1]
[1] https://github.com/llvm/llvm-project/commit/93f602b339f62e058ce2c212c86f9f0ca3b20126
@sameeranjoshi, the llvm commit process recommends that you add the Differential Revision message to your commit message. This will also automatically close the patch in review.
https://llvm.org/docs/Phabricator.html#committing-a-change