This patch treats <filesystem> as a first-class citizen of the dylib,
like all other sub-libraries (e.g. <chrono>). As such, it also removes
all special handling for installing the filesystem library separately
or disabling part of the test suite from the lit command line.
Details
- Reviewers
mclow.lists EricWF serge-sans-paille - Commits
- rGcc37af7a3631: [libc++] Build <filesystem> support as part of the dylib
rCXX356518: [libc++] Build <filesystem> support as part of the dylib
rL356518: [libc++] Build <filesystem> support as part of the dylib
rG72122d058b17: [libc++] Build <filesystem> support as part of the dylib
rCXX356500: [libc++] Build <filesystem> support as part of the dylib
rL356500: [libc++] Build <filesystem> support as part of the dylib
Diff Detail
- Repository
- rL LLVM
Event Timeline
Note that we need https://reviews.llvm.org/D59093 before we take this patch. Otherwise, all back-deployment CI on Apple platforms is going to go red.
libcxx/CMakeLists.txt | ||
---|---|---|
79 ↗ | (On Diff #189918) | Is it OK to turn fielssytem on for WIN32? |
libcxx/CMakeLists.txt | ||
---|---|---|
79 ↗ | (On Diff #189918) | Good point. If not, I assume it's just a temporary situation until support is implemented. If that's correct, then I would disable <filesystem> for WIN32 using some other means and have a TODO or something like that. If there's a deeper reason, then we may want to consider keeping the ability to have filesystem turned off (but I doubt that). |
libcxx/utils/libcxx/test/config.py | ||
---|---|---|
710 ↗ | (On Diff #189918) | There are going to be users and platforms that don't supply filesystem and wish to disable the tests. How can they do that? |
libcxx/utils/libcxx/test/target_info.py | ||
244 ↗ | (On Diff #189918) | This change seems dangerously incorrect. When testing the dylib, we don't want to link any libraries not linked by clang by default. Please keep this as it -lrt as it was. |
This LGTM other than the inline nits.
Let me know if you want me to produce the linux ABI list changes.
PS. It would be really nice if we found an easier way to generate all the ABI list changes for a given change at once.
Yes, that would be nice.
PS. It would be really nice if we found an easier way to generate all the ABI list changes for a given change at once.
Agreed, but even better would be to use an explicit list of symbols to export. This way, we wouldn't need to _test_ for what symbols are exported, and the list of symbols would be naturally maintained alongside the library.
libcxx/utils/libcxx/test/config.py | ||
---|---|---|
710 ↗ | (On Diff #189918) | I view this like <chrono>. If an implementation doesn't support <filesystem> for some reason, I can create a lit feature like dylib-has-no-filesystem in the availability patch and then mark the filesystem tests as unsupported when that lit feature is provided. That's the way we handle partial implementations for other sub-libraries, and I think it should be the way we do it for <filesystem> too. |
libcxx/utils/libcxx/test/target_info.py | ||
244 ↗ | (On Diff #189918) | So what I did was only substitute enable_fs for True because in the case of libc++ this will always be true, and then I was able to remove the if statement altogether. SO we're already always linking against -lrt in all tests when enable_filesystem=True today. I think the _actual_ problem is the FIXME above, where we should be more granular in linking -lrt only when for <filesystem> tests. Does that make sense? If you agree with that, I'm happy to work on fixing that FIXME as a follow-up patch. |
libcxx/utils/libcxx/test/target_info.py | ||
---|---|---|
244 ↗ | (On Diff #189918) | The check was essentially checking if we had any static libraries. It checked for filesystem because filesystem was always static. When it's in the dylib the explicit -lrt shouldn't be needed. Let's address the FIXME and only link -lrt when not shared_libcxx. |
For some reason, all the filesystem tests were removed when I updated the diff to fix Eric's comment about -lrt.
Un-remove all the filesystem tests. I have no idea how this happened in the first place.
libcxx/trunk/docs/UsingLibcxx.rst | ||
---|---|---|
53 | Post commit comment: We shouldn't have removed this documentation. Instead it should be documented to say that after 8.0 filesystem is in the dylib and libc++fs.a doesn't exist. |
libcxx/trunk/docs/UsingLibcxx.rst | ||
---|---|---|
53 | You are right -- see r356681. |
libcxx/trunk/utils/libcxx/test/config.py | ||
---|---|---|
710–713 | This appears to break CUDA buildbot: Apparently lit does not find that directory, even though it *is* present in the checked out tree with this config file. I guess lit has somehow misdetected libcxx_src_root, but I see nothing relevant in the logs. Any ideas what could be wrong? |
libcxx/trunk/utils/libcxx/test/config.py | ||
---|---|---|
710–713 | No idea what could be wrong, however can you check on the bot whether <...>/libcxx/test/std/input.output/filesystems/Inputs/static_test_env exists? This seems to be what lit is complaining about. |
libcxx/trunk/utils/libcxx/test/config.py | ||
---|---|---|
710–713 | That directory does exist. I've tracked the weirdness down to what may be a problem with get_lit_conf(): def get_lit_conf(self, name, default=None): val = self.lit_config.params.get(name, None) if val is None: val = getattr(self.config, name, None) if val is None: val = default return val The problem is that when libcxx/test/config.py is used from libunwind's lit.cfg, getattr(self.config, 'libcxx_src_root', None) there actually appears to returns an empty string. No idea who/where/how sets it. It's possible that the problem is somewhere in libunwind. Anyways, comparison with None fails, the empty string is returned as the result and things go downhill from there. |
libcxx/trunk/utils/libcxx/test/config.py | ||
---|---|---|
710–713 | I guess the empty string is the result of this line in various lit.site.cfg.in : config.libcxx_src_root = "@LIBCXX_SOURCE_DIR@" Perhaps if val is None should be changed to if not Val when we check result of getattr(self.config) as the empty string there will be a common default for unset parameters. |
libcxx/trunk/utils/libcxx/test/config.py | ||
---|---|---|
710–713 | I've temporarily worked around the problem by changing projects/libunwind/test/lit.site.cfg.in on the buildbot and hardcoding the path to libc++ source tree: config.libcxx_src_root = "/the/path/to/the/libcxx/sources" I still don't understand lit enough to tell what's the right way to get this working on my buildbot. |
Post commit comment:
We shouldn't have removed this documentation. Instead it should be documented to say that after 8.0 filesystem is in the dylib and libc++fs.a doesn't exist.