[PATCH] D96371: [lit] Add --ignore-fail

Fri Feb 12 10:21:12 PST 2021

jdenny added a comment.

In D96371#2559299 <https://reviews.llvm.org/D96371#2559299>, @jhenderson wrote:

> In D96371#2556413 <https://reviews.llvm.org/D96371#2556413>, @thopre wrote:
>
>> I'm still not sure, I think a failure is a powerful motivator to fix something but patch LGTM otherwise. @jhenderson what do you think?
>
> Sorry, I'm a little bit busy with other reviews, so haven't had a chance to dig into this properly yet.
>
> If I follow what you're saying correctly, rather than check-all executing lit in a single run across multiple directories, in some cases, it is spawning a new lit process to run some test subset. Is that right?

Yes.  In the current case, that subset is for runtimes specified in `LLVM_ENABLE_RUNTIMES`.  I believe the last case I saw was `compiler-rt/test/gwp_asan/unit`, but I think that one has since been fixed.

> If so, I wonder if there's a better solution of getting check-all to actually run all the tests as one big test.

Agreed.

However, especially after these responses, I see `--ignore-fail` as a temporary workaround.  By "temporary", I mean until the cmake config issue is fixed.  I see it as a permanent lit feature because this isn't the first time our cmake config has had this issue, and I'm betting it won't be the last time.  Otherwise, I wouldn't have proposed this patch upstream.

In D96371#2556413 <https://reviews.llvm.org/D96371#2556413>, @thopre wrote:

> I'm still not sure, I think a failure is a powerful motivator to fix something

Good point.  Even so, I think there is still motivation to fix such cmake issues: because the exit status is zero when using `--ignore-fail`, trying to detect failures in a script can be ugly.

I've updated the review summary to account for everyone's feedback, which has honed my view.  Thanks.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96371