This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use named Lit features to flag back-deployment XFAILs
ClosedPublic

Authored by ldionne on Mar 18 2023, 7:42 AM.

Details

Summary

Instead of writing something like XFAIL: use_system_cxx_lib && target=...
to XFAIL back-deployment tests, introduce named Lit features like
availability-shared_mutex-missing to represent those. This makes the
XFAIL annotations leaner, and solves the problem of XFAIL comments
potentially getting out of sync. This would also make it easier for
another vendor to add their own annotations to the test suite by simply
changing how the feature is defined for their OS releases, instead
of having to modify hundreds of tests to add repetitive annotations.

This doesn't touch *all* annotations -- only annotations that were widely
duplicated are given named features (e.g. when filesystem or shared_mutex
were introduced). I still think it probably doesn't make sense to have a
named feature for every single fix we make to the dylib.

This is in essence a revert of 2659663, but since then the test suite
has changed significantly. Back when I did 2659663, the configuration
files we have for the test suite right now were being bootstrapped and
it wasn't clear how to provide these features for back-deployment in
that context. Since then, we have a streamlined way of defining these
features in features.py and that doesn't impact the ability for a
configuration file to stay minimal.

The original motivation for this change was that I am about to propose
a change that would touch essentially all XFAIL annotations for back-deployment
in the test suite, and this greatly reduces the number of lines changed
by that upcoming change, in addition to making the test suite generally
better.

Diff Detail

Event Timeline

ldionne created this revision.Mar 18 2023, 7:42 AM
ldionne requested review of this revision.Mar 18 2023, 7:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2023, 7:42 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik accepted this revision.Mar 18 2023, 8:46 AM
This revision is now accepted and ready to land.Mar 18 2023, 8:46 AM
ldionne updated this revision to Diff 506311.Mar 18 2023, 1:10 PM

Fix incorrect markup in libcxx/test/std/strings/basic.string/string.capacity/reserve_size.pass.cpp

ldionne updated this revision to Diff 506332.Mar 18 2023, 2:15 PM

Fix a couple more incorrect XFAILs

Mordante accepted this revision.Mar 19 2023, 11:13 AM
Mordante added a subscriber: Mordante.

I spot checked the patch, but trust on the CI to catch issues.

There's one question, other than that LGTM.

libcxx/utils/libcxx/test/features.py
357

Since vendors can disable filesystem, can't you tie into the same feature?

ldionne marked an inline comment as done.Mar 22 2023, 11:19 AM
ldionne added inline comments.
libcxx/utils/libcxx/test/features.py
357

Yeah, that was on my mind too, but for this patch I wanted to really just capture the availability failures. The has-no-filesystem feature is a bit different in intent -- it says whether the vendor has enabled the filesystem library, which is a bit different.

So basically, yes I want to reconsider this part of the filesystem enablement, but I think this approach is right for the scope of this patch.

ldionne updated this revision to Diff 507491.Mar 22 2023, 1:45 PM
ldionne marked an inline comment as done.

Rebase

ldionne updated this revision to Diff 507540.Mar 22 2023, 4:11 PM

Rebase for CI

ldionne updated this revision to Diff 507732.Mar 23 2023, 7:01 AM

Rebase to re-trigger CI

ldionne updated this revision to Diff 508319.Mar 25 2023, 9:35 AM

Rebase to trigger CI (which seems to be back online)

ldionne updated this revision to Diff 508330.Mar 25 2023, 11:38 AM

Rebase for CI.

ldionne updated this revision to Diff 508340.Mar 25 2023, 2:00 PM

Fix typo in backdeployment annotation

ldionne updated this revision to Diff 508620.Mar 27 2023, 6:13 AM

Fix typo in availability comment again and temporarily disable CI other than macOS for testing.

ldionne updated this revision to Diff 508625.Mar 27 2023, 6:29 AM

Re-upload to try and re-trigger CI

ldionne updated this revision to Diff 508627.Mar 27 2023, 6:37 AM

Remove all jobs except the Apple ones

ldionne updated this revision to Diff 508704.Mar 27 2023, 9:43 AM

Re-upload without temporary CI changes. I'll land this since it passed all the jobs.

This revision was landed with ongoing or failed builds.Mar 27 2023, 9:44 AM
This revision was automatically updated to reflect the committed changes.