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

Eric Fiselier via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Mar 19 08:42:48 PDT 2019

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

  rCXX libc++



More information about the libcxx-commits mailing list