[llvm] r268048 - cmake: Set LINK_POLLY_INTO_TOOLS to ON (v2)

[Justin Bogner via llvm-commits]

Tobias Grosser <tobias at grosser.es> writes:
> Hi Justin,
>
> thanks for reporting.
>
>> I'm not totally sure that this works correctly. I'm seeing some weird
>> behaviour in clean builds, at the very least. It seems like this
>> *sometimes* fails and tries to link polly into bugpoint even if you
>> don't have it checked out, but I'm having trouble reproducing so I might
>> be confused.
>
> This is definitely surprising and unexpected. Do you still see this?
> Does this same behavior arise consistently after cmake has built the
> make/ninja files or do we have some non-deterministic behavior in a
> parallel make/ninja build?
>
>>> +if(WITH_POLLY)
>>> +  if(NOT EXISTS ${LLVM_MAIN_SRC_DIR}/tools/polly/CMakeLists.txt)
>>> +    set(WITH_POLLY OFF)
>>> +  endif()
>>> +endif(WITH_POLLY)
>>> +
>>> +if (NOT WITH_POLLY)
>>> + set(LINK_POLLY_INTO_TOOLS OFF)
>> 
>> Is this usage of set() correct? IIUC it creates a new non-cached
>> variable that happens to have the same name as the option above. At the
>> very least, I see both WITH_POLLY and LINK_POLLY_INTO_TOOLS set to on in
>> my CMakeCache after configuring with this, despite not having polly
>> checked out.
>
> I think this is correct. The behavior intended was that the user facing
> options are not changed (in case the user checks out Polly later on),
> but to ignore these options and to disable the Polly build in case Polly
> has not been checked out. As the newly created non-cache variable will
> hide the older cache variable [2] this seems to be exactly what is
> happening.

Okay, I figured out what my problem was. If you start with a clean build
after this change, then checkout something older and reuse the build
directory (say because you switched branches or are trying to bisect a
problem), *then* you get a tree that doesn't build.

This is because before these changes, the variables being set to on in
the cache if you didn't have polly checked out did the wrong thing.

This is unfortunate but not horrible, and it seems hard to fix, so it's
probably fine.

> Now, I agree that this is clearly unintuitive. I will see if I can come
> up with something better and post it for review.
>
>> I don't really know how option() works compared to set(... CACHED) though.
>
> I could not find any specific point of documentation, but from the cmake
> mailing list [1] it seems that option(VAR "") is effectively the same as
> SET(VAR OFF CACHE BOOL "...").