- User Since
- Aug 20 2014, 4:35 PM (256 w, 4 d)
Sat, Jul 20
Adding ‘FORCE’ makes this not user overridable, which is the whole point of this being a cache variable.
Wed, Jul 17
Thu, Jul 11
This is fine with me. I have no real attachment to the name.
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.
Tue, Jul 9
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.
Mon, Jul 8
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?
Sun, Jul 7
Fri, Jul 5
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.
Tue, Jul 2
One comment inline. Otherwise LGTM.
Jun 20 2019
I really don't think this is the right solution.
Jun 10 2019
Jun 5 2019
Jun 3 2019
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>.
May 31 2019
This is good to land.
May 30 2019
May 29 2019
Looks reasonable to me.
May 28 2019
May 24 2019
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.
I have the same concerns as @labath, so we should certainly put it behind a flag.
May 23 2019
@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.
Makes sense to me.
May 22 2019
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.
May 21 2019
@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?
@sbc100 they do not reproduce for me locally. They are Linux-specific and I’m on Darwin.
That conflict is because of how I implemented libLLVM, and is something I'm looking to change. I'm hoping to put up a patch this week that will update how we generate libLLVM to be more like how libclang_shared is built.
Adding "libcxxabi" to LLVM_ENABLE_RUNTIMES is fine, but the other changes to the DistributionExample are only needed because you chose to use gold, which is a configuration-specific decision that is not representative of how most people will build, therefore it shouldn't be in the example.
One small change needed, otherwise LGTM.
It is silly that CMake doesn't expose the ninja version, but it is easy enough to get it. It should be something like:
This has had plenty of time to percolate, so I'm going to land it shorty. Any additional feedback we can incorporate in follow-up patches.
@E5ten, I just pushed r361271, which should resolve your issue.
@E5ten, I see what is going on. Give me 30 minutes and I’ll have a fix pushed.
May 20 2019
I have a long story about this issue... Ask me about it sometime :).
@hintonda also, re-running the CMake command line and setting -D... is always a FORCE
@thakis Didn't we patch Ninja for this?
The CMake goop all looks reasonable to me. I think the new "correct" way to alter the compiler flags is add_compile_options, but since LLVM pretty heavily relies on modifying CMAKE_<LANG>_FLAGS it is probably better to be consistent than to start doing something new here.
Thank goodness @smeenai can spell because obviously I can't.
Looping in @EricWF.
A new take on how to do this, even more fun this time!
I just came up with a different and slightly crazier approach to this. I'm going to test it out and will upload a new version of this patch shortly.
Updates based on feedback from @smeenai
Updates based on review feedback.
Adding @zturner in case he has any thoughts.
@winksaville I just committed an update to the DistributionExamples a minute ago that should get them working as long as you aren't using a Darwin host. If you are using a Darwin host you need D62155 as well.
May 19 2019
@winksaville, sorry those cache files pre-date the monorepo, and they haven’t been updated to support it. I will try and get a patch to update them today.
May 18 2019
The main thing is to make sure that if you have proper integer sized members they are properly aligned and packed, which this seems to do well.
May 17 2019
This is pretty cool. Since these are usually allocated as globals, this is saving pages of dirtied memory.
LGTM. Makes sense.
I would leave it out of any distribution (at least for now).
@hans I have some ideas on how to make package work. There is a simple solution to make it work as long as you're not using any runtime components, but I have an idea on how to make it work with runtime components.
Fixing the wording around LLVM_INSTALL_TOOLCHAIN_ONLY