Page MenuHomePhabricator

[libcxxabi] Fewer assumptions about path from libcxx to libcxxabi
ClosedPublic

Authored by thakis on Feb 24 2021, 6:28 AM.

Details

Reviewers
rnk
ldionne
Group Reviewers
Restricted Project
Restricted Project
Commits
rG72b18a86e11e: [libcxxabi] Fewer assumptions about path from libcxx to libcxxabi
Summary

This is useful for projects that pull in libcxx and libcxxabi and build
them using out-of-tree build files, but don't make them sibling
directories (or don't call the sibling directories libcxx and libcxxabi
for some reason).

Fixes PR49313.

Diff Detail

Event Timeline

thakis created this revision.Feb 24 2021, 6:28 AM
thakis requested review of this revision.Feb 24 2021, 6:28 AM

(Alternatively, the include could say "../src/include/refstring.h"
since libcxx/include is already added as an include path, but it
sounds like there's a desire to make it get includes from the
build directory instead of the source directory in the future.
So I didn't go with that.)

rnk added a subscriber: ldionne.Feb 24 2021, 11:20 AM

This looks fine to me, but I feel like we should get approval of this from @ldionne.

ldionne accepted this revision.Feb 25 2021, 11:39 AM

I'm fine with this change per-se, however the problem I have is with Chrome swimming against the current in terms of having the runtimes in non-standard locations.

We introduced this requirement a year ago: https://lists.llvm.org/pipermail/libcxx-dev/2020-March/000714.html. There were even suggestions about merging libc++ and libc++abi, which is something I know Eric has been wanting to do for a long time.

We can make this change, but it would be best for Chrome to change the way it's currently ingesting libc++ and libc++abi to account for those requirements. By the way, I think we had an explicit error at the top of the CMake that would have fired for using libc++/libc++abi in non-standard locations relative to each other, but I removed it some time ago (apparently before you updated your version of the libs).

The bottom line is that we want to be able to assume that we are in a monorepo-like layout in libc++ and libc++abi because it simplifies things, and in the future we might very well depend on some things like a common set of CMake utilities outside of either libcxx/ or libcxxabi/. If you change how you ingest libcxx/libcxxabi right now and ingest the monorepo instead (or at least a subset of the directories in the monorepo, right now only libcxx/ and libcxxabi/ are needed to build either), you won't risk being broken again in the future.

This revision is now accepted and ready to land.Feb 25 2021, 11:39 AM

Thanks!

I'm fine with this change per-se, however the problem I have is with Chrome swimming against the current in terms of having the runtimes in non-standard locations.

We introduced this requirement a year ago: https://lists.llvm.org/pipermail/libcxx-dev/2020-March/000714.html. There were even suggestions about merging libc++ and libc++abi, which is something I know Eric has been wanting to do for a long time.

We can make this change, but it would be best for Chrome to change the way it's currently ingesting libc++ and libc++abi to account for those requirements. By the way, I think we had an explicit error at the top of the CMake that would have fired for using libc++/libc++abi in non-standard locations relative to each other, but I removed it some time ago (apparently before you updated your version of the libs).

Understood.

From what I understand, the main benefits from assuming this is that it makes the cmake stuff easier since you can share more cmake modules etc. Chromium doesn't use the cmake files for libc++(abi), so if the cmake files make this assumption that isn't a problem for us. (Just to be clear: Chromium also doesn't use the gn files in llvm's tree, instead it has some build files downstream for libc++(abi), like for all its other third-party dependencies). I think it's fairly common for projects to deps in source code and have custom build bits. I think that's true for Firefox and Android too, for example. So I think as long as libc++ and libc++abi are distinct projects, making it possible to compile the source files of each in distinct locations is generally useful (as long as it doesn't require big contortions, which in this case it doesn't).

If libc++ and libc++abi would merge into a single repo, that wouldn't be a problem for us either.

The bottom line is that we want to be able to assume that we are in a monorepo-like layout in libc++ and libc++abi because it simplifies things, and in the future we might very well depend on some things like a common set of CMake utilities outside of either libcxx/ or libcxxabi/. If you change how you ingest libcxx/libcxxabi right now and ingest the monorepo instead (or at least a subset of the directories in the monorepo, right now only libcxx/ and libcxxabi/ are needed to build either), you won't risk being broken again in the future.

Making this assumption in cmake files isn't a problem for us. Just making that assumption in source files.

Thanks again!

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 26 2021, 6:10 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
thakis edited the summary of this revision. (Show Details)Feb 26 2021, 7:26 AM