[libcxx-commits] [PATCH] D91574: [libc++] Simplify how we pick the typeinfo comparison

[Jonas Devlieghere via Phabricator via libcxx-commits]

JDevlieghere added a comment.

In D91574#2405909 <https://reviews.llvm.org/D91574#2405909>, @ldionne wrote:

> In D91574#2405856 <https://reviews.llvm.org/D91574#2405856>, @JDevlieghere wrote:
>
>> [...]
>>
>> LLVM has a bunch of incremental bots and in addition to trying to be be fast, they also mirror what developers do when they pull. I can only speak for myself, but I certainly don't remove the CMake cache every time I do that. I've been involved in a bunch of CMake-related code reviews and as this is a pretty common problem, which I've seen people address by introducing a temporary CMake variable to let the bots (and developers) catch up. Is there any reason that couldn't have worked here?
>
> The only trick I'm aware of is to temporarily use `FORCE` in the CMake `set()` command to force the cache to be overwritten. I've used that trick in the past to avoid exactly this kind of breakage, however in this case that would also break any build that explicitly sets that CMake variable (i.e. `-DLIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION=XXX`). So the reason why this broke bots is not that I didn't care about it, it's that I thought it was inevitable.
>
> Now that you mention a temporary variable, I think I know where you're going. Do you mean:
>
> 1. Introduce a new cache variable with a different name, and make the code use it. That new variable should be initialized to the old one if it's set, to accommodate bots that set the old name explicitly.
> 2. `set(FORCE)` the old variable name to whatever you want.
> 3. Wait for the bots to catch up (like 1 day or so).
> 4. Move the code back to the original variable name, which will now have the correct default set by the `set(FORCE)`.
> 5. Delete the temporary variable name, which is not useful anymore.
>
> That sounds like it *might* work, however it's kind of complicated too, no?

Yep, at least that's how we dealt with this for LLDB in the past.

> It seems to me like it would be better to acknowledge that CMake caches are invalidated from time to time, and make sure our infrastructure is robust against that.

I totally agree. Some of the bots allow you to specify a variable (which admittedly is totally obscure) to request a clean build which is something else I've used in the past (when I had already landed something without the workaround and wanted to avoid reverting).

>> FWIW if you disagree with how the bots deal with incremental builds, you're of course free to change that, but in the meantime the burden is still on the author of the patch and anyone could've reverted this in accordance with LLVM's "revert-to-green" policy.
>
> I don't disagree with how they deal with incremental builds -- I just think we should be fine with a bit of human intervention from time to time if we're going to assume the cache never gets invalidated. And at the same time, I suggested a technical approach (full build + `ccache`) to make the bots more robust so that these issues don't happen again in the future.

I think that's fair, but if you expect something like this to break, it might be nice to land it at a time where you can work with the bot owners to get it resolved quickly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91574/new/

https://reviews.llvm.org/D91574