Closed msteinert closed 7 years ago
Thanks for the PR. I'm not entirely sure static linking is a safe "default" or if it breaks other users. Would it be possible to make static linking optional?
These changes shouldn't force anyone to statically link their application, just allow them to successfully link with a static libfolly.
Do you have a specific concern? I can go through the changes line-by-line if it would be helpful. I guess typically this configuration would come from pkg-config, etc.
@anirudhvr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Adding find_paths makes it definitely better but it can be made more robust. You can also add hints to allow the specification of paths on build time with flags, i.e. -DGLOG_PATH="~/Desktop/glog". For now that's fine/
For atomic check:
Actually, on a second check. I don't think atomic is needed. The three platforms Wangle has been tested on (AMD64, AARCH64, and PPC64) should all have lock-free atomics and shouldn't need libatomic to be linked in.
This is essentially the same test that Folly performs in its configure script.
The architectures you listed do support atomic operations. I have tested the following platform combinations:
My understanding is that the compiler provides support for std::atomic
through libatomic, which is distributed as a part of GCC. The GCC Wiki explains it in more detail.
Consider the following program:
#include <atomic>
int main() {
struct Test { int val; };
std::atomic<Test> s;
s.is_lock_free();
}
System information:
$ lsb_release -a
LSB Version: core-9.20160110ubuntu0.2-amd64:core-9.20160110ubuntu0.2-noarch:security-9.20160110ubuntu0.2-amd64:security-9.20160110ubuntu0.2-noarch
Distributor ID: Ubuntu
Description: Ubuntu 16.04.2 LTS
Release: 16.04
Codename: xenial
$ g++ --version
g++ (Ubuntu 5.4.1-2ubuntu1~16.04) 5.4.1 20160904
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Compile without libatomic:
$ g++ -std=c++11 test.cc -o test
/tmp/cc5o0C4Y.o: In function `std::atomic<A>::is_lock_free() const':
test.cc:(.text._ZNKSt6atomicI1AE12is_lock_freeEv[_ZNKSt6atomicI1AE12is_lock_freeEv]+0x19): undefined reference to `__atomic_is_lock_free'
collect2: error: ld returned 1 exit status
Compile with libatomic:
$ g++ -std=c++11 test.cc -o test -latomic
$ echo $?
0
I definitely need to do some more manual testing to check this more in depth. Anyways, this is good work! Thanks!
@eduardo-elizondo has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
I was trying to compile a static-only version of this library that links with static versions of the dependencies. A few things seem to be missing from the CMake configuration to make this possible.
The extra find_path commands allow for the (admittedly uncommon) situation where every dependency is installed in a separate location.