[PATCH] D68833: [CMake] Re-order runtimes in the order of dependencies

[Louis Dionne via Phabricator via llvm-commits]

ldionne added a comment.

In D68833#1706296 <https://reviews.llvm.org/D68833#1706296>, @beanz wrote:

> In D68833#1706104 <https://reviews.llvm.org/D68833#1706104>, @ldionne wrote:
>
> > CMake tracks dependencies between targets, but not between directories. If the CMakeLists.txt in some directory (e.g. `<root>/libcxx/`) needs a target defined in another directory (e.g. `<root>/libcxxabi/`), one has to make sure that `libcxxabi`'s `CMakeLists.txt` is included before `libcxx`'s `CMakeLists.txt`. This isn't new or vexing, IMO. If that is what this patch ensures (I don't know the runtimes build very well), I think this is good.
>
>
> [...]
>  I'm not sure what you mean by this. What I *think* you are referring to is specifying runtime libraries in `LLVM_ENABLE_PROJECTS` which uses the llvm/projects subdirectory for building them.


Yes, precisely. That's also the currently preferred way of building libc++: https://libcxx.llvm.org/docs/BuildingLibcxx.html

> This build flow is also part of the monorepo and uses `LLVM_ENABLE_RUNTIMES`. Fundamentally `LLVM_ENABLE_PROJECTS` is the incorrect way to build runtime libraries because they are not built with the in-tree compiler. While this works for most development cases it is 100% wrong for building and shipping toolchains, and it is questionable practice to have developers building and testing things that aren't representative of what we ship.

Not everybody ships libc++/libc++abi as part of the toolchain, and for those, building with whatever `CMAKE_CXX_COMPILER` they specify is really the right thing to do. Don't get me wrong, I'm 100% on board that there's value in having this runtime build, however let's not pretend that it's the only correct way to build libc++.

> 
> 
>> I don't see how `HAVE_${runtime}` can get around things like being able to query properties of e.g. `cxxabi_shared` inside the libcxx build before `cxxabi_shared` has been defined. I do support a push towards using generator expressions more, but I don't think generator expressions are a complete solution to this problem.
> 
> What do you think that generator expressions can't do that is relevant to this problem?

Say I need to generate a file based on the properties of a target. I'll need to call `get_target_property` on a target that hasn't been defined yet, and there's no way around that because `file(GENERATE)` does not expand generator expressions.

> 
> 
>> I'd like to see this patch go in under some form so that we can remove the hacky workaround introduced in https://reviews.llvm.org/D68791.
> 
> You can also remove that hacky workaround using generator expressions.

Are you thinking about this?

  set(libname "$<IF:$<TARGET_EXISTS:${lib}>,$<TARGET_PROPERTY:${lib},OUTPUT_NAME>,${lib}>")
   list(APPEND link_libraries "${CMAKE_LINK_LIBRARY_FLAG}${libname}")

That is clever, I had not thought about it.

If the above workaround works, I don't care about this patch that much. I still think we need to clarify the status of the Runtimes build and document it, unless that's already done and I've missed it (in which case please point it to me).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68833