[libcxx-commits] [libcxx] Reverts around time_zone (PR #95058)

[Vitaly Buka via libcxx-commits]

vitalybuka wrote:

> As mentioned before, the sanitizer bots have false positives every now and then. I agree with @ldionne that a time-out in a bot is not per-se an error in our code.

In this case it was ~10 consecutive failures, which is definitely not a flake.

I will be very concerned about health of the project if we start treating bot issues as not "out code".

> Reverting and relanding is not always trivial. Temporary disabling tests on configurations that do not work is a solution we use in libc++.

Yes. But most of the time it's trivial.
Temporarily disabling is absolutely fine. But that's not the point. We should not think twice before reverting.

> We try to do better, in libc++ we have a pre-commit CI to test all our supported configurations.

CI does best effort but can't cover all supported configurations. E.g. I don't see anything except x86.

> My personal issue with reverting directly is that it sometimes is easier to add a patch that temporary disables a configuration.

That all true, but reverting should still be treated as trivial thing.
Sorry, here I made wrong call about bug or not, suspecting deadlock, but it's not the point. If we revert, after day or so of  debugging, or your insight about long test, we'd reland and move on. Note this was landed in about a week after approval, I don't buy that 1-2 days will delay will make a difference. 

> Especially since buildbot maintainers sometimes don't have time to look at an issue in a timely fashion. That means development of a feature is blocked until that can be resolved.

"Blocked" is exaggeration. We all work we a lot of branches and commits in local checkouts.

>  (Using stacked patches in GitHub, unlike in Phabricator, is quite painful. This is already makes development of larger features harder than previously.)

https://github.com/getcord/spr not the same but pretty close.

BTW. It would be nice to at least pause landing the stack after the firs bot email.

> Note that in cases where I expect issues in the code due to CI issues I revert my patches. 

If you believed that this is CI issue from the beginning, it could be helpful to comment. Somehow it didn't click for me from the beginning that this is just a long test, and not deadlock, which means a bug in the compiler or libcxx.

But again it's not the point. If we do reverts easily, we had this patch re-landed with test disabled in no time, without the drama.

https://github.com/llvm/llvm-project/pull/95058