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

Deal with the case Boost_LIBRARY_DIRS is empty #1378

Merged
merged 2 commits into from Dec 4, 2019

Conversation

xuning97
Copy link
Contributor

@xuning97 xuning97 commented Dec 3, 2019

deal with possible Boost_LIBRARY_DIRS empty case,
which happens when I use a local compiled boost 1.71.0 installed to /usr/locl/lib.

deal with possible Boost_LIBRARY_DIRS empty case,
which happens when I use a local compiled boost 1.71.0 installed to /usr/locl/lib
set_target_properties(calendar PROPERTIES LINK_FLAGS "-L${Boost_LIBRARY_DIRS}")
if (${Boost_LIBRARY_DIRS})
set_target_properties(calendar PROPERTIES LINK_FLAGS "-L${Boost_LIBRARY_DIRS}")
endif()
Copy link
Owner

Choose a reason for hiding this comment

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

How does this work? The calendar target needs to link to the Boost.DateTime and Boost.ProgramOptions libraries. Does it even make sense to build the calendar target if those libraries aren't there?

Copy link
Contributor

@JohelEGP JohelEGP Dec 3, 2019

Choose a reason for hiding this comment

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

This checks if Boost_LIBRARY_DIRS is set. I guess the local installation of @xuning97 doesn't include a CMake package. In other words, it relies on the compiler to find them. For calendar to not work in that case, it'd have to use modern CMake, which would mean linking against the Boost:: targets, rather than this manual manipulation of properties.

Copy link
Owner

Choose a reason for hiding this comment

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

I still don't get how Boost_FOUND can be true but Boost_LIBRARY_DIRS is empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe if it uses Boost CMake, assuming it doesn't set it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

It is set by FindBoost.cmake, which is a standard part of every CMake install: https://github.com/Kitware/CMake/blob/master/Modules/FindBoost.cmake

Copy link
Contributor

Choose a reason for hiding this comment

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

As stated in the link above for Boost CMake, the logic does use the installed Boost, which does not set Boost_LIBRARY_DIRS.

Copy link
Contributor Author

@xuning97 xuning97 Dec 4, 2019

Choose a reason for hiding this comment

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

There is no Boost_LIBRARY_DIRS in the cmake file shipped with boost sources.
https://github.com/boostorg/boost_install/blob/develop/BoostConfig.cmake

Even if -L/path/to/boost/lib is not specified, it's possible to complete the link as long as the boost libraries are in the standard lib path or specified by LD_LIBRARY_PATH.
But if Boost_LIBRARY_DIRS is empty, the link command becomes -L{empty input}, which leads to strange link error information.

@@ -33,6 +33,8 @@ if(RANGES_BUILD_CALENDAR_EXAMPLE)
message(STATUS "boost: ${Boost_LIBRARY_DIRS}")
target_compile_definitions(calendar PRIVATE BOOST_NO_AUTO_PTR)
target_compile_options(calendar PRIVATE -std=gnu++14)
set_target_properties(calendar PROPERTIES LINK_FLAGS "-L${Boost_LIBRARY_DIRS}")
if (${Boost_LIBRARY_DIRS})
set_target_properties(calendar PROPERTIES LINK_FLAGS "-L${Boost_LIBRARY_DIRS}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
set_target_properties(calendar PROPERTIES LINK_FLAGS "-L${Boost_LIBRARY_DIRS}")
target_link_libraries(calendar PRIVATE Boost::date_time Boost::program_options)

Based on the discussion on the line below, this is how it should look like. Discard the other changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a master in this area, Can this type of change support older version of boost?

Copy link
Contributor

Choose a reason for hiding this comment

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

What actually matter is the CMake support for the Boost targets, which were added in CMake 3.5. Range-v3 requires 3.6, so it's alright.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this PR.

Use modern way to link with boost libraries, so there is no need to
specify -L directly
@ericniebler ericniebler merged commit 50ea6ac into ericniebler:master Dec 4, 2019
@ericniebler
Copy link
Owner

Very nice, thank you.

brevzin pushed a commit to brevzin/range-v3 that referenced this pull request Jan 1, 2020
In some case Boost_LIBRARY_DIRS is empty, which happens when I use a local compiled boost 1.71.0 installed to /usr/locl/lib

Instead, use modern way to link with boost libraries, so there is no need to
specify -L directly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants