Page MenuHomePhabricator

cmake: Make building clang-shlib optional
Needs ReviewPublic

Authored by jvesely on Aug 11 2019, 8:39 PM.

Details

Reviewers
beanz
compnerd
Summary

It takes ~5min to link, add an option to avoid that.

Diff Detail

Repository
rC Clang

Event Timeline

jvesely created this revision.Aug 11 2019, 8:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2019, 8:39 PM
Herald added a subscriber: mgorny. · View Herald Transcript

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 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've no problem with it staying ON by default. I just don't see a reason to make every minor modification rebuild last 5-6mins instead of 30s, for something I don't want, need or use.
Not everyone is a billion-dollar corporation with a dedicated build farm.

beanz added a comment.Aug 12 2019, 2:58 PM

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.

If you *really* *really* can't be bothered to ensure that the things we ship actually build with your change you can always use the (undocumented, and hidden) CLANG_TOOL_CLANG_SHLIB_BUILD=Off option. I really don't see a reason to add a user-facing option that we don't want people using.

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.

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.

If you *really* *really* can't be bothered to ensure that the things we ship actually build with your change you can always use the (undocumented, and hidden) CLANG_TOOL_CLANG_SHLIB_BUILD=Off option. I really don't see a reason to add a user-facing option that we don't want people using.

I'm pretty sure more people would appreciate not installing large duplicate libraries, but if a hidden option will do, I'll update the patch.

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.

This is a really odd workflow, and not at all how our project is designed to be used. We have CMake exports explicitly to allow building against a build directory instead of needing to install.

I'm pretty sure more people would appreciate not installing large duplicate libraries, but if a hidden option will do, I'll update the patch.

Running make install is massively contrary to this stated issue. Our install target generally installs pretty much everything, which is likely duplicating libraries. If you can't change your workflow to not rely on running install, I'd suggest you use the LLVM_DISTRIBUTION_COMPONENTS option (http://llvm.org/docs/BuildingADistribution.html), to hand tailor what pieces of LLVM you actually care about. That will give you the most optimal build cycle.

E5ten added a subscriber: E5ten.Aug 13 2019, 10:19 AM

I am in favour of adding a user-facing option to disable generating this duplicate library for users that don't need it, like @jvesely says, there should be an option to disable linking a library that takes a long time to link and isn't necessary for a lot of users.

I want to dissect this a bit.

I am in favour of adding a user-facing option to disable generating this duplicate library for users that don't need it

Why do you call this duplicate? It is unique. There is no other library in the clang build that serves the role of this library.

there should be an option to disable linking a library that takes a long time to link and isn't necessary for a lot of users.

I think this is nuanced. When you say "takes a long time to link", I'm curious why you say that. For me it takes 45s to link on my laptop in a Linux VM using LLD in a build configuration that also includes all our backends (which is kinda a worst-case scenario), and 10s to link if I only include X86. That doesn't seem like a super long time, and it doesn't rely on any billion-dollar compute farms.

While it does slow down full-build times (slightly), I think the benefit is less broken bots which benefits the community as a whole.

Going back to @jvesely's original email, I'm not sure why this adds minutes to your build time. I'd be curious if there are other low-hanging fruit that would improve your productivity without the community cost of adding new build configurations that disable building and testing things that we actually ship.

I want to dissect this a bit.

I am in favour of adding a user-facing option to disable generating this duplicate library for users that don't need it

Why do you call this duplicate? It is unique. There is no other library in the clang build that serves the role of this library.

It duplicates functionality provided by separate/component libraries.
Why can't this be an option the same way I can pick when building llvm?

there should be an option to disable linking a library that takes a long time to link and isn't necessary for a lot of users.

I think this is nuanced. When you say "takes a long time to link", I'm curious why you say that. For me it takes 45s to link on my laptop in a Linux VM using LLD in a build configuration that also includes all our backends (which is kinda a worst-case scenario), and 10s to link if I only include X86. That doesn't seem like a super long time, and it doesn't rely on any billion-dollar compute farms.

Is this a debug build?

While it does slow down full-build times (slightly), I think the benefit is less broken bots which benefits the community as a whole.

do you have any numbers to support that claim?

Going back to @jvesely's original email, I'm not sure why this adds minutes to your build time. I'd be curious if there are other low-hanging fruit that would improve your productivity without the community cost of adding new build configurations that disable building and testing things that we actually ship.

My first guess would be the difference between debug and release build (presence of debug info). the size of the library is 1.6GB on my system:

$ du -h /usr/local/llvm-git/lib/libclang-cpp.so.10svn 
1.6G	/usr/local/llvm-git/lib/libclang-cpp.so.10svn

just reading the inputs and writing the output library will take 30s on a 100MB/s hdd (my laptop is slower than that) and it hasn't done any linking yet.

It duplicates functionality provided by separate/component libraries.

The component libraries can't be used the way this can, and this is intended as an installed target, whereas the clang component libraries aren't. These are distinct, calling it duplicate is inaccurate and not helpful to the conversation. It may be that you don't need this library, or want it, but that is different.

Why can't this be an option the same way I can pick when building llvm?

I'm going to come back to this below.

Is this a debug build?

It was RelWithDebInfo, so that probably makes a big difference since it significantly reduced the amount of debug info being linked.

do you have any numbers to support that claim?

Sadly we don't keep logs of 5 years worth of build breakages. Since I added the LLVM shared library build, we've had a large number of breakages because people don't build it, many of the breakages at configuration time.

One of the big problems we have today with the LLVM build system is that the number of different configurations is simply massive. I certainly don't have a full understanding of all the configuration options people are using and how they are using them, and I've spent more time working on the LLVM build system than most other contributors. The variety of different configurations which conflict or overlap with each other has resulted in the build system being hard to maintain and modify.

If we continue to allow each engineer to add options to the build system to make their workflow streamlined we will continue adding options until our CMake becomes completely unmaintainable. For that reason some of us have been talking about a new approach to maintaining the build system. The clang-shlib change was one of the first examples of the new approach in action. The general idea of the new approach is to not add new configurations. Instead we want to encourage people to use existing mechanisms in the build system to improve their workflow and iteration times, and I believe we have everything you need already.

This does pre-suppose that engineers are willing to alter their workflows slightly to rely on different CMake options and build targets. I don't think that is an unreasonable position to come from, and frankly I think the inverse (that LLVM should adapt to each individual engineer's workflow) is unreasonable.

In your workflow, rather than building make install, if you use the CMake LLVM_DISTRIBUTION_COMPONENTS option to specify which parts of LLVM & Clang you need, you can run make install-distribution (similarly rather than make, make distribution). This gives you tighter control than any other mechanism over what you build and install, and is the preferred way to do what you are trying to do (not build some parts of a project).

Hi,

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.
That said, forcing everyone to build one huge library effectively makes debug builds unusable in any practical way.
Having a usable debug build would also obsolete the with_asserts option.
What is the use-case of one big shared lib that split libraries can't do?

beanz added a comment.Aug 20 2019, 3:51 PM

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.

This is exactly what we want to avoid in the future by simplifying build configurations.

That said, forcing everyone to build one huge library effectively makes debug builds unusable in any practical way.

Nobody is forced to build this. You can run the check targets which don't include this library in their dependency chain, and you can use the LLVM_DISTRIBUITON_COMPONENTS option I mentioned earlier to create build and install targets that exclude it. We are just including it as part of the all target, because it is part of building everything. Where I think you and I are really disagreeing is that you seem resistant to altering your workflow.

What is the use-case of one big shared lib that split libraries can't do?

clang-cpp is an installable and shippable library, which is not the use case that the split libraries were designed for. Also, since it is a shared library, tools can be linked against it to reduce binary distribution sizes (at the cost of some runtime performance). It is worth noting that the BUILD_SHARED_LIBS option is explicitly documented in several places as being for developer use only, not for shipping. Which means this is the only supported mechanism for shipping clang's logic in a shared library.