Page MenuHomePhabricator

[libc++] Build <filesystem> support as part of the dylib
ClosedPublic

Authored by ldionne on Mar 8 2019, 1:35 PM.

Details

Summary

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.

Event Timeline

ldionne created this revision.Mar 8 2019, 1:35 PM

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.

jfb added inline comments.Mar 8 2019, 1:44 PM
libcxx/CMakeLists.txt
79

Is it OK to turn fielssytem on for WIN32?

ldionne marked an inline comment as done.Mar 8 2019, 1:50 PM
ldionne added inline comments.
libcxx/CMakeLists.txt
79

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).

EricWF added inline comments.Mar 8 2019, 8:31 PM
libcxx/utils/libcxx/test/config.py
710

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
242–243

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.

EricWF added a comment.Mar 8 2019, 8:33 PM

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.

ldionne marked 2 inline comments as done.Mar 11 2019, 8:50 AM

This LGTM other than the inline nits.

Let me know if you want me to produce the linux ABI list changes.

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

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
242–243

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.

E5ten added a subscriber: E5ten.Mar 13 2019, 9:30 AM
EricWF added inline comments.Mar 19 2019, 8:38 AM
libcxx/utils/libcxx/test/target_info.py
242–243

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.

ldionne updated this revision to Diff 191315.Mar 19 2019, 8:48 AM
ldionne marked an inline comment as done.

Address Eric's comment with -lrt.

EricWF accepted this revision.Mar 19 2019, 12:06 PM

LGTM.

This revision is now accepted and ready to land.Mar 19 2019, 12:06 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2019, 12:10 PM
ldionne reopened this revision.Mar 19 2019, 12:53 PM

For some reason, all the filesystem tests were removed when I updated the diff to fix Eric's comment about -lrt.

This revision is now accepted and ready to land.Mar 19 2019, 12:53 PM
ldionne updated this revision to Diff 191374.Mar 19 2019, 12:56 PM

Un-remove all the filesystem tests. I have no idea how this happened in the first place.

ormris removed a subscriber: ormris.Mar 19 2019, 1:49 PM
This revision was automatically updated to reflect the committed changes.
EricWF added inline comments.Mar 20 2019, 3:17 PM
libcxx/trunk/docs/UsingLibcxx.rst
53 ↗(On Diff #191391)

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.

ldionne marked 2 inline comments as done.Mar 21 2019, 9:20 AM
ldionne added inline comments.
libcxx/trunk/docs/UsingLibcxx.rst
53 ↗(On Diff #191391)

You are right -- see r356681.

tra added a subscriber: tra.Apr 2 2019, 1:40 PM
tra added inline comments.
libcxx/trunk/utils/libcxx/test/config.py
710–713 ↗(On Diff #191391)

This appears to break CUDA buildbot:
http://lab.llvm.org:8011/builders/clang-cuda-build/builds/31842/steps/ninja%20check%201/logs/stdio

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?

ldionne marked 2 inline comments as done.Apr 2 2019, 2:06 PM
ldionne added inline comments.
libcxx/trunk/utils/libcxx/test/config.py
710–713 ↗(On Diff #191391)

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.

tra added inline comments.Apr 2 2019, 2:30 PM
libcxx/trunk/utils/libcxx/test/config.py
710–713 ↗(On Diff #191391)

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.

tra added inline comments.Apr 2 2019, 2:42 PM
libcxx/trunk/utils/libcxx/test/config.py
710–713 ↗(On Diff #191391)

I guess the empty string is the result of this line in various lit.site.cfg.in :
llvm/projects/libcxx/test/lit.site.cfg.in
llvm/projects/libunwind/test/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.

tra added inline comments.Apr 2 2019, 5:33 PM
libcxx/trunk/utils/libcxx/test/config.py
710–713 ↗(On Diff #191391)

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.