Do we need to worry about compatibility? What happens if there is a mix between an old binary and a new binary?

I'm not sure I understand the motivation for this change. Can you please expand on that? How are you using it that it doesn't work for you? Additionally, z is definitely the wrong name, please compute that by using get_file_name_component, CMAKE_SHARED_LIBRARY_PREFIX, and string(REGEX...)

Hmm, I see, yes, that would change the behavior where we would not create the targets for it. In that case, using EXCLUDE_FROM_ALL is fine, but set it as a target property rather than passing global variables like this. But, even better would be to understand if we even need this.

GIT 68a235d07f9

GIT 2046d72e916

Should probably add a check for __block variables.

The std::transform better expresses the intent IMO. I think that it is definitely more idiomatic C++, so I think that is an improvement.

@stella.stamenova ping

LGTM with the additional test for the size/type mismatch validation.

In D70764#1767395, @JDevlieghere wrote:

Having one canonical variable controlling zlib support seems indeed desirable.

In D70519#1754618, @labath wrote:

With this patch, what is the output of llvm-config --system-libs ?

@compnerd What's the answer to this for this patch?

There isn't a workaround for this, its just a bug in our CMake. I've fixed the issue with our CMake logic in GIT 372ad32734ecb455f9fb4d0601229ca2dfc78b66.

The commit message doesn't really explain much. Why simply consume the argument? Why not actually halt the build since this clearly is conflicting, and its unclear what the user intended.

Could you please provide a proper commit message before committing this? Also -Xifs is actually the inverse of what you want - that would be a set of arguments designed to be passed to llvm-ifs.

@labath I think you are misunderstanding the patch. This is not autoselecting the dependencies. It is simply doing that based on an existing option that we have - LLVM_ENABLE_ZLIB. We could always search for zlib and override the results with LLVM_ENABLE_ZLIB as well. The current build will continue to just work - zlib is used only for the compressed debug sections (which requires the user to opt-in).

GIT 340e7c0b77a

I think that rather than adding more wrapping macros, we should fix the structure. We can mark the examples directory from ALL at the add_subdirectory level and only add the subdirectory when examples are enabled. This avoids the need for the new macro. I think that we want to move towards a more standard cmake build rather than a more specialized one.

Use @labath's suggestion of bumping minimum required version for Windows

Sorry about the delay on the review!

The test adjustments are incorrect. They should optionally accept the triple in the path.

I'm kinda torn on this. Although this simplifies the build, linking the DSO on Linux is painful unless you externalise the debug info (i.e. build with fission). On the other hand, people should be encouraged to setup the distributions and use that to build the full set that they need. I think that marking libLLVM as EXCLUDE_FROM_ALL is a decent compromise.

GIT rb314414570c

I agree with @efriedma that it sounds odd, could you explain that please?

Can you please write a more complete commit message explaining the need for this.

Seems like a good idea since lld link2 has been made into lld link for a while now.

Yeah, doing an incremental rollout makes sense.

The reason for bringing this back up as a Windows specific thing is that currently, there is no good way to build LLDB with python support without having to specify additional details on *just* windows because the windows path is doing something special. This is trying to bring the windows path to parity with the Linux path.