This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add a test to pin down the set of transitive public includes
ClosedPublic

Authored by ldionne on Jun 20 2022, 5:51 PM.

Details

Summary

A situation that happens fairly often in libc++ is that we remove some
transitive includes in a header (either purposefully or not) and that
ends up breaking users. Of course, we want to be able to remove our
transitive includes, however it's also good to have a grip on that
to know which commit changed what and when. Furthermore, it's good
to accumulate include removals for a couple of releases to avoid
breaking users at every release for this reason.

This commit adds a test that should break whenever we remove an
include. Hence, it should allow us to track which headers include
which other headers transitively, giving us a traceable way to
remove headers.

Diff Detail

Event Timeline

ldionne created this revision.Jun 20 2022, 5:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2022, 5:51 PM
ldionne requested review of this revision.Jun 20 2022, 5:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2022, 5:51 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne updated this revision to Diff 438712.Jun 21 2022, 8:23 AM

Poke CI, which is rather flaky right now.

Thanks for working on this! I think this is really useful to have. The makes it easier to remove includes from subheaders and see whether that as a user visible impact.
I'm a bit curious whether it will cause a lot of build failures. (I know it shouldn't.)

libcxx/test/libcxx/transitive_includes.sh.cpp
16

Would it be an idea to add a "schedule" here too? Basically to avoid people working on a cleanup patch and have it rejected just because it's not "the right time". For example only cleanup when the next version number is a multiple of 3. (3 just because 15 is a multiple of 3.)
(But maybe we should discuss the details on Discord.)

libcxx/test/libcxx/transitive_includes/expected.algorithm
2

Should the path include c++/v1? If we want that we probably need to make the test unsupported for the unstable ABI too.

ldionne updated this revision to Diff 439058.Jun 22 2022, 9:30 AM
ldionne marked 2 inline comments as done.

Do not list c++/v1/ explicitly.

libcxx/test/libcxx/transitive_includes.sh.cpp
16

When I first read this comment, I was thinking "this is a great idea". Then I gave it some more thought and I am not so sure. Indeed, I don't think we should be held to a specific (and arbitrary) schedule for breaking transitive header dependencies: instead we should do it in a reasoned fashion when it makes the most sense. For example, we are going to ship large parts of Ranges in LLVM 15, so making the transition to LLVM 15 as seamless as possible is desirable for users to get the benefits of Ranges ASAP. So much so that my plan was actually to go back and re-add some headers we've removed since branching LLVM 14 to avoid breaking folks in this release.

After all, I don't think we would reject any cleanup patch because it's "not the right time". We would simply add some transitive includes for backwards compatibility with a TODO above them, so the cost here is really small.

TLDR: Let's be very intentional about removing these transitive dependencies, and we can even give a heads up in advance for people to get prepared. The amount of breakage that this can cause (and the barrier it adds to adoption of a new libc++) is surprisingly high.

Mordante accepted this revision as: Mordante.Jun 22 2022, 10:17 AM

LGTM when the CI is happy.

libcxx/test/libcxx/transitive_includes.sh.cpp
16

I started a discussion on Discord.

jloser added a subscriber: jloser.Jun 22 2022, 11:37 AM

I like this general direction! Thanks for working on this.

ldionne updated this revision to Diff 439136.Jun 22 2022, 12:45 PM

Try to fix CI.

ldionne updated this revision to Diff 439173.Jun 22 2022, 3:08 PM

More CI fixes, in particular try to make the test work on Windows and AIX by ditching grep and sed.

ldionne updated this revision to Diff 439485.Jun 23 2022, 11:39 AM
ldionne added subscribers: daltenty, mstorsjo.

XFAIL on Windows and AIX. This should be close to working on Windows and AIX (I tried removing anything obviously non-portable).

@mstorsjo @daltenty When I land this, it would be really good to have someone fix the test on those two platforms.

ldionne accepted this revision.Jun 23 2022, 1:23 PM
This revision is now accepted and ready to land.Jun 23 2022, 1:23 PM