Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy of FindGflags.cmake in cmake/ and minor compilation fixes in tests #28

Closed
wants to merge 3 commits into from

Conversation

ccicconetti
Copy link
Contributor

On Ubuntu 16.04 cmake 3.10.3 does not find the FindFlags.cmake unless it is copied in the cmake/ directory, which in fact already contains other cmake find scripts.

Changes to unit tests are very minor only to allow compilation with gcc 5.4.0 (not tested with other compilers).

Consistent with other CMake find scripts, otherwise cmake 3.10.3 does
not find it.
Member function call of test class not found inside a lambda, unless
this is specified explicitly with g++ 5.4.0.
Test class must defined explicitly.
EXPECT_EQ() with Boolean values transformed into EXPECT_TRUE()/EXPECT_FALSE(), preventing a compiler warning (g++ 5.4.0).
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jul 18, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lnicco has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@lnicco
Copy link
Contributor

lnicco commented Jul 19, 2019

thanks @ccicconetti ;)
FindGflags needs some work indeed, and there is also a similar PR for folly facebook/folly#1170
We have a more generic fix in the pipeline that should fix it for all the fb projects.
So for now we may just take the fixes to the tests, and wait for the GFlags fix.
how does that sound?

@ccicconetti
Copy link
Contributor Author

Very good, indeed. Duplicating the FindGflags script never looked like a permanent solution to me, as well, but rather a work-around to get through compilation. Glad to know there is ongoing work on this.

Actually, the worst part for me was to stubbornly try to build with cmake 3.5.1 (latest version shipped on Ubuntu 16.04), which works fine with folly and fizz, but not mvfst. Eventually I had to give up and install a local copy of a newer version of cmake, which went smooth. In Ubuntu 18.04 all was OK first try.

@lnicco
Copy link
Contributor

lnicco commented Jul 20, 2019

yeah mvfst currently has some dependencies on cmake >= 3.10

quic/public_root/CMakeLists.txt:cmake_minimum_required(VERSION 3.10)

@udippant knows more about that.

The system closes the PR automatically on commit.
Please reopen if something is still missing.

@facebook-github-bot
Copy link
Contributor

@lnicco merged this pull request in 2a97e61.

facebook-github-bot pushed a commit that referenced this pull request Mar 14, 2022
Summary:
X-link: facebook/fb303#28

X-link: facebook/fboss#115

X-link: facebook/folly#1736

X-link: facebook/proxygen#403

X-link: facebook/fbthrift#488

This adds a way to pass arguments to the `b2` build tool, used by Boost. This is needed in order to link a getdeps built boost into an relocatable `.so`. The motivating use case is that we need to statically link Boost into a native library used by a python wheel, which must be relocatable. This functionality already exists for CMake-based projects.

Reviewed By: mackorone

Differential Revision: D34796774

fbshipit-source-id: 0d6a9f4703865dc02048b87e77394c44ef646af6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants