Do not explicitly depend on llvm tools during standalone build

[serge-sans-paille]

Summary

When building lldb in standalone mode, the llvm-nm, llvm-objdump etc target are not available, so only conditionnaly include them.

[xiaobai]

I think you might need to rebase this patch because D57233 seems to accomplish part of what this patch does, but I don't see those changes here.

[labath]

Alex and Stefan are the standalone build experts these days, so I'll let them do the reviewing here.

[sgraenitz]

All these targets are exported from the LLVM build-tree since D56606 and from the install-tree since D57383.

I did not revert or continue to work on D57233 yet, because it needs a good concept first. LLDB_BUILT_STANDALONE is not the right condition here. What we really want to know is:

• Have LLVM tools/utils been built (LLVM_BUILD_TOOLS/UTILS) when we have a build-tree.
• Have LLVM tools/utils been installed (LLVM_INSTALL_TOOLS/UTILS) when we have an install-tree.
• What tree do we have? I have seen hacks to determine it, but not sure whether there's a clean way.

[sgraenitz]

@xiaobai && @compnerd Do you know if find_package can tell us anything about it?

[compnerd]

@sgraenitz - not really ... the find_package will load the LLVMConfig.cmake that is generated. We only have what we keep in there.

[xiaobai]

@sgraenitz: I don't think find_package alone will help you here. LLVMConfig doesn't necessarily tell you what's actually been built and/or installed, but it can tell you what's exported. Here's what I would do:

1. List the tools you need for LLDB_TEST_DEPS from LLVM
2. For each tool, check to see if it's exported from the LLVM CMake package (this is as simple as checking if it's a target)
3. If it's not exported, proceed to the next tool
4. If it is exported, use find_program to check if it was built/installed (LLVMConfig should tell you where it is -- LLVM_TOOLS_BINARY_DIR)
5. If it is built, add it to LLDB_TEST_DEPS.
6. If it's not built, don't add it to LLDB_TEST_DEPS.

I think this should work regardless of if you have a build tree or an install tree. I don't see any logic in here that is specific to one or the other, but if you absolutely do need to check, probably the most reliable way is to check for the existince of the LLVM_BUILD_* variables. I think the install tree also has a variable LLVM_INSTALL_PREFIX, so that could be another way to check.

[labath]

BTW, what's the reason for having super-high-fidelity list of dependencies in a standalone build? I know that they're important for in-tree builds, as that makes sure everything is built when you run "check-lldb". However, that's isn't going to help you with standalone builds. The binaries are already there, or they aren't, and there's nothing we can do to change that. I'd consider just removing all non-lldb targets from the deps list when in standalone mode.

[xiaobai]

In D58193#1398376, @labath wrote:

BTW, what's the reason for having super-high-fidelity list of dependencies in a standalone build? I know that they're important for in-tree builds, as that makes sure everything is built when you run "check-lldb". However, that's isn't going to help you with standalone builds. The binaries are already there, or they aren't, and there's nothing we can do to change that. I'd consider just removing all non-lldb targets from the deps list when in standalone mode.

I think it's useful to be able to fully test lldb if you have all the available tools even if it's built standalone. This is especially relevant for swift-lldb, which more often than not is built standalone because swift is often built standalone. I'm generally not in favor of keeping things around only because somebody downstream uses it, but I can see the usefulness for people who work on upstream LLDB as well. I'm not sure if other people feel the same way though.

[labath]

In D58193#1398458, @xiaobai wrote:

I think it's useful to be able to fully test lldb if you have all the available tools even if it's built standalone. This is especially relevant for swift-lldb, which more often than not is built standalone because swift is often built standalone. I'm generally not in favor of keeping things around only because somebody downstream uses it, but I can see the usefulness for people who work on upstream LLDB as well. I'm not sure if other people feel the same way though.

I certainly agree with that, but I don't see how fiddling with LLDB_TEST_DEPS is going to help testing at all. AFAICT, all it does it set's up the dependencies in the cmake graph. That is not going to help you run the tests. If the tools are there, the tests will run fine; if they aren't the tests will fail.

Now if we were talking about detecting the available tools and automatically warning about their absence and/or skipping tests, that would be a different story, but I don't see anyone proposing that...

[sgraenitz]

In D58193#1398376, @labath wrote:

BTW, what's the reason for having super-high-fidelity list of dependencies in a standalone build?

There's a lot of ways to build LLDB (in-tree, standalone, monorepo, single-/multi-config generator) in a lot of contexts (OSs, against build/install-tree) and multiple test frameworks with different requirements (python, lit, unittests). We use a load of conditions to handle all this and they complicate the CMake configuration. Some of these build on wrong assumptions (as we see here with LLDB_BUILT_STANDALONE or earlier with LLDB_TEST_C/CXX_COMPILER) that always require long explanations. We never test everything and often use quick-fixes that have the tendency to add new quirks. Flakiness of tests add confusion on top of it.

IMHO using targets directly in both, in-tree vs. standalone builds, would be a good first step in streamlining the configuration process. We could warn/error early on missing dependencies. We could use generator expressions consistently to query paths of external binaries, Clang headers, etc.

In D58193#1399059, @labath wrote:

Now if we were talking about detecting the available tools and automatically warning about their absence and/or skipping tests, that would be a different story, but I don't see anyone proposing that...

That's part of it, yes.

[labath]

Ok, I see. Thanks for explaining that. I agree that it would be great if we could reduce the differences between an in-tree and a standalone build to a minimum.

[sgraenitz]

Yap, will try and come up with a proposal soon.

@serge-sans-paille Sorry for diverging the discussion so much. Is it working for you now or are you still eager to get this patch in?

[serge-sans-paille]

@sgraenitz I currently have this patch applied to LLVM 8rc1 source tree for fedora, because it wasn't working automagically otherwise. Reading the discussion, I don't think I missed some configuration stuff, or what did I miss when configuring the build of lldb in standalone mode?

[labath]

In D58193#1401137, @serge-sans-paille wrote:

@sgraenitz I currently have this patch applied to LLVM 8rc1 source tree for fedora,

Could you check what's the state of the master branch? It's possible some of the changes we've been talking about didn't make the 8.0 cut. We still have the time to cherry-pick it over, but it's going to run out soon.

[sgraenitz]

In D58193#1401137, @serge-sans-paille wrote:

@sgraenitz I currently have this patch applied to LLVM 8rc1 source tree for fedora, because it wasn't working automagically otherwise. Reading the discussion, I don't think I missed some configuration stuff, or what did I miss when configuring the build of lldb in standalone mode?

You build against an installed LLVM and you want to run LLDB tests? AFAIK you need to:

• configure LLVM with LLVM_INSTALL_UTILS=ON
• configure standalone LLDB with LLVM_EXTERNAL_LIT=/path/to/llvm-build-tree/bin/llvm-lit

However, llc and dsymutil are tools and they should not be affected.
If you don't want to run the test suite, pass LLDB_INCLUDE_TESTS=OFF.

Maybe @mgorny can add some info here?

[sgraenitz]

@serge-sans-paille After all, it appears to me that your issue is caused by something else, but if this change fixes it, I am not against taking it for now. It shouldn't do any harm. Please get in touch with @hans if this is really relevant for release_80.

However, please note that your patch base is not up-to-date with master [1] or release_80 [2] branches. The meanwhile changes shouldn't affect the dsymutil or llc targets, but maybe it's a hint towards the actual problem.
Also note that the lines you want to modify are there since May [3] and September [4] respectively, so it's been this way already in the 7.0 release.

(I am off till early March and will only check my inbox rarely. Hope everything's gonna work out well.)

[1] https://github.com/llvm-mirror/lldb/blob/master/lit/CMakeLists.txt
[2] https://github.com/llvm-mirror/lldb/blob/release_80/lit/CMakeLists.txt
[3] https://reviews.llvm.org/rL331447#change-GVDgVmL1F2CA
[4] https://reviews.llvm.org/rL342733#change-GVDgVmL1F2CA

[serge-sans-paille]

@sgraenitz : it's possible that r352382 fixes my issue. I'll double check that first!

[serge-sans-paille]

@sgraenitz It's fixed! I'm dropping this patch then, thanks for investigating o/

lit/CMakeLists.txt

 get_property(LLDB_DOTEST_ARGS GLOBAL PROPERTY LLDB_DOTEST_ARGS_PROPERTY)
 
 string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_LIBS_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
 string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TOOLS_DIR ${LLVM_RUNTIME_OUTPUT_INTDIR})
 string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_DOTEST_ARGS "${LLDB_DOTEST_ARGS}")
 
 list(APPEND LLDB_TEST_DEPS
   LLDBUnitTests
-  dsymutil
-  llc
   lldb
   lldb-test
+  )
+
+if (NOT LLDB_BUILT_STANDALONE)
+  list(APPEND LLDB_TEST_DEPS
+    dsymutil
+    llc
     llvm-config
     llvm-mc
     llvm-objcopy
     FileCheck
     count
     not
     )
+endif()
 
 if(TARGET lld)
   list(APPEND LLDB_TEST_DEPS lld)
 endif()
 
 # the value is not canonicalized within LLVM
 llvm_canonicalize_cmake_booleans(
   LLDB_DISABLE_PYTHON