[libcxx-commits] [PATCH] D59152: [libc++] Build <filesystem> support as part of the dylib

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 2 inline comments as done.
ldionne added a comment.

In D59152#1423668 <https://reviews.llvm.org/D59152#1423668>, @EricWF wrote:

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



================
Comment at: libcxx/utils/libcxx/test/config.py:710
     def configure_filesystem_compile_flags(self):
-        enable_fs = self.get_lit_bool('enable_filesystem', default=False)
-        if not enable_fs:
----------------
EricWF wrote:
> There are going to be users and platforms that don't supply filesystem and wish to disable the tests.
> 
> How can they do that?
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.


================
Comment at: libcxx/utils/libcxx/test/target_info.py:244
         if enable_threads:
-            flags += ['-lpthread']
-            if not shared_libcxx or enable_fs:
-              flags += ['-lrt']
+            flags += ['-lpthread', '-lrt']
         flags += ['-lc']
----------------
EricWF wrote:
> 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.
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.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59152/new/

https://reviews.llvm.org/D59152