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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added inline comments.


================
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']
----------------
ldionne wrote:
> 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.
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`.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D59152