In D67585#1670491, @mgorny wrote:

But in that case, we should aim for consistency, i.e. remove the matching option from LLVM.

In D67585#1670433, @mgorny wrote:

This is really much more work than disabling the one component I don't need or care for, especially when I do shared lib build and therefore the additional library is entirely useless.

In D67585#1670420, @mgorny wrote:

Sure but I want to build-test practically everything else. I wouldn't mind if this didn't basically kill my test system by resource exhaustion

Please no. You don’t need to build the library. ‘check-clang’ doesn’t depend on it, so it should not impact your build and test cycles. We want it included in the ‘all’ target for every possible configuration so that it gets tested and not broken as frequently as the LLVM dylib target has in the past.

Updates based on feedback from Lang.

@aaronpuchert sounds like a reasonable approach

I see. Sorry for the noise. This looks fine.

@compnerd, it doesn't look like LLVM_ENABLE_RUNTIMES passed globally is still passed through if you are generating runtimes for multiple targets. It looks like that only gets passed through if you specify RUNTIMES_${name}_LLVM_ENABLE_RUNTIMES.

I feel like the default behavior should be that if you set LLVM_ENABLE_RUNTIMES it builds them for all targets you request. If you want to disable some runtimes for certain targets that should be an opt-out setting rather than an opt-in to build for targets.

Landed in rL369486

I'm happy to commit this for you. Will do so shortly.

Seems reasonable to me. @anemet any concerns?

In D66068#1636240, @jvesely wrote:

sorry for the delay. I fully understand the need to reduce the number of options. Having always used SHARED_LIBS build I remember weekly shared build breakages.

In D65983#1633766, @cbiesinger wrote:

It actually worked with the env for me.

Sadly I'm not sure that there is a better way than generating the shebang line, but I can imagine some problems with the way this patch does it. PYTHON_EXECUTABLE could be a full path, which means that this can't be used to generate binary distributions because you'd be relying on python being in the same place on disk, which defeats the purpose of env being on the shebang.

This fix looks good, although I would suggest that when building libcxx, using the LLVM CMake exports should be more reliable than llvm-confit.

In D66068#1628617, @jvesely wrote:

It duplicates functionality provided by separate/component libraries.

I want to dissect this a bit.

In D66068#1626266, @jvesely wrote:

That's your workflow. I need to run 'make install' because the modifications are used by an external project. If there's an option to skip shlib during 'make install' I'd be happy to use it.

I think you and I disagree here. General developer workflows don't need to include building all for every minor change. In my normal workflow I just re-run check-llvm or check-clang over and over again, only building the all target before I post a patch. With that workflow I only build the library once per-patch to ensure that it builds. Which is exactly the goal of not having it be included.

I generally am not a fan of adding more and more options. As long as you're not linking the library it won't be generated by any of the check targets. With the llvm dylib we've had many issues over the years where changes to LLVM break building the dylib and many developers don't build it, so we have to wait for a bot to catch it. Making the clang dylib build as part of all in every possible build configuration is a recognition that this is something we ship and we should ensure works.

I don't know of any way to emulate that because it isn't something CMake exports or even knows about. It is something the underlying build system detects from the compiler's depfiles. One thought that might work would be to generate an object library target containing the tool source files, and instead depend on the object library and all the target static archives. I think that would get the ordering right and would not require the link step to run.

You'd need to make the NATIVE target depend on all the source files and headers that the the target depends on, which is theoretically possible, although I'm not sure of a clean way to do it in CMake.

This patch is effectively un-done as of rL367895. I'm happy to discuss alternative approaches to the problem you're trying to solve, but we need correct builds, which means we need the dependency between the native and target tool.

This patch is actually really problematic. We have the native tablegen depend on the target tablegen so that the native one rebuilds whenever the sources for the target one rebuild. Without that dependency the native tablegen may not rebuild at all, or (if it does) it will likely rebuild at the wrong time.

Yes please! I actually have this locally.

In D65295#1602891, @Meinersbur wrote:

If this is the proper way, why do we even support add_llvm_library(${NAME} MODULE without PLUGIN_TOOL?

LGTM.

LGTM. At some point (after the mono-repo becomes the source of truth) we need to clean all this up.

The CMake build system option PLUGIN_TOOL on add_llvm_loadable_module is for this specific purpose, it instructs the link step for the plugin dylib to resolve symbols against the tool.

Adding ‘FORCE’ makes this not user overridable, which is the whole point of this being a cache variable.

This is fine with me. I have no real attachment to the name.

LGTM.

Yea, this makes sense even if it is a bit sad. We do use the --whole-archive approach for libLLVM, and it works. I was hoping to avoid it here in part to free up the ability to link libclang_shared before or in parallel to archiving the libclang*.a archives.

LGTM.

One thought, we should probably evaluate why those tools are using add_library instead of add_llvm_library. The right fix might be to move them over. This is exactly the kind of behavior that add_llvm_library and llvm_add_library were designed to handle.

I don't like the idea of putting this in llvm_update_compile_flags. That function does exactly what its name says, and adding additional behavior to it seems undesirable. Where are we creating libraries with add_library that we need this?

Reading through the old comments on this thread I think there are a few things going on here, but the core of the issue is you're using BUILD_SHARED_LIBS *outside* LLVM's build system. That is explicitly not supported (https://www.llvm.org/docs/CMake.html). We do not support BUILD_SHARED_LIBS for distributing LLVM in any form.

This is definitely not the right fix. We don’t want to set everything as Public dependencies. Our build system’s model for dependencies is that each component specified what it directly uses which is its Private dependencies. If you are having issues that probably means missing dependencies.

One comment inline. Otherwise LGTM.

I really don't think this is the right solution.

LGTM

I have a thought for how we can make the CMake interface for this better. Instead of LLVM_EXTENSIONS being a global property if it were a pre-defined list like LLVM_ALL_PROJECTS, then the LLVM_LINK_<X>_INTO_TOOLS variables can be generated by LLVM's configuration instead of relying on Polly's to run. If you do that LLVM_LINK_POLLY_INTO_TOOLS could be set to On even if Polly isn't enabled to build, but that actually will be fine if you change the calls to target_link_libraries in the tools to use a generator expression something like: $<$<TARGET_EXISTS:Polly>,Polly>.

This is good to land.

Looks reasonable to me.

In D62353#1516322, @hintonda wrote:

Btw, where would you want to put them?

Pre-monorepo, it would depend on what the bot is building, but with the mono-repo we could just create a new top-level directory to contain them all. Name each file to match the bot config it runs.

LGTM.

I have the same concerns as @labath, so we should certainly put it behind a flag.

@winksaville I've figured out how to resolve the gtest issue, but unfortunately that isn't good enough to get the check-runtimes target working. A change went in back in February (rL353601), which breaks running compiler-rt's tests when building a distribution in non-trivial ways, which will unfortunately be difficult to resolve.

I know this is an old change, but there are actually some big problems with this. Compiler-RT in particular uses COMPILER_RT_STANDALONE_BUILD=Off to basically mean clang is a target in-tree to depend on. This completely breaks being able to run the compiler-rt tests from the runtimes directory because a bunch of the tests add dependencies on clang. See clang_compile and its uses in CompilerRTCompile.cmake.

In D62279#1514467, @winksaville wrote:

Even with both gtest patches it still isn't working, ninja check-all is failing as before :(

Makes sense to me.

In D62279#1514046, @winksaville wrote:

I've add gtest_main and gtest to CLANG_BOOTSTRAP_TARGETS as a guess, because that's where check-all is defined:
...
My guess is likely wrong, what do you advise?

In D62215#1512849, @winksaville wrote:

IIRC, on linux if LTO is ON I had to use ld.gold for both stage 1 and 2, although that maybe
just the way it ended up and it wasn't necessary. But I'll test whatever you come up with.

A very interesting idea. A few thoughts inline.

Fixing this patch

Just came to my attention this explodes on some versions of CMake. I'm looking at it and will update once I find a better approach.

D62269 should fix the missing dependency on gtest.

In D62215#1512543, @winksaville wrote:

With this error I guessed that I needed to build LLVMgold.so, but then I also determined
I needed to link everything with ld.gold to complete the build process, which is why I
uploaded this patch.

@sbc100, I think I see what I did wrong in my last attempt at a fix. I was reading the wrong CMake property. rL361334 should resolve that. Can you give it a try and let me know if that resolves the problem for you?