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

add fcl/0.6.1 #1263

Merged
merged 1 commit into from
Apr 16, 2020
Merged

add fcl/0.6.1 #1263

merged 1 commit into from
Apr 16, 2020

Conversation

SpaceIm
Copy link
Contributor

@SpaceIm SpaceIm commented Apr 3, 2020

Specify library name and version: fcl/0.6.1

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

#621

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Apr 3, 2020

Just to mention two things;

  • MinGW builds might fail, even with -Wa,-mbig-obj (in Debug mode there is little hope).
  • Does self.deps_cpp_info["octomap"].version return the true version of octomap in the dependency tree (eventually overriden by consumer?)

@conan-center-bot
Copy link
Collaborator

Some configurations of 'fcl/0.6.1' failed in build 1 (998d20d53e39150756f1bc0e5534fa32aaa4b6d0):

recipes/fcl/all/conanfile.py Outdated Show resolved Hide resolved
@SpaceIm SpaceIm force-pushed the fcl/0.6.1 branch 2 times, most recently from f5aee35 to c3875eb Compare April 4, 2020 07:42
@conan-center-bot
Copy link
Collaborator

All green in build 3 (c3875eb257e069d95a933748982034b6ef207ffe)! 😊

@conan-center-bot
Copy link
Collaborator

All green in build 4 (e08bd64652372fcdcd6bae3d536cbce4cef2f2e5)! 😊

SSE4
SSE4 previously approved these changes Apr 6, 2020
@SSE4 SSE4 requested a review from uilianries April 6, 2020 07:43
@SSE4
Copy link
Contributor

SSE4 commented Apr 6, 2020

  1. MinGW is okay to be added at later point. for now, you may just throw ConanInvalidConfiguration, or leave as is
  2. yes, I think so, e.g. the same approach worked well in OpenCV recipe

uilianries
uilianries previously approved these changes Apr 6, 2020
@SpaceIm
Copy link
Contributor Author

SpaceIm commented Apr 6, 2020

It works for MinGW as static lib, so point 1 is not longer relevant.

I've also a huge patch to fix shared lib on Windows (MinGW and Visual Studio), but honestly, I can't put that in the recipe (it exceeds size limit for one recipe). I'll submit a PR to upstream. It was hard and tedious to fix.

@madebr
Copy link
Contributor

madebr commented Apr 6, 2020

I've also a huge patch to fix shared lib on Windows (MinGW and Visual Studio), but honestly, I can't put that in the recipe (it exceeds size limit for one recipe). I'll submit a PR to upstream. It was hard and tedious to fix.

That would be nice!
In my experience, adding a shared build to the test matrix (.appveyor.yml) will avoid breaking this in the future and makes upstream more willing to accept these patches.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Apr 9, 2020

I've submitted a PR: flexible-collision-library/fcl#461
As is, it will be rejected, because it bumps CMake minimum version to 3.7. Could be modified a little bit with a less elegant solution in order to preserve CMake version.

@madebr
Copy link
Contributor

madebr commented Apr 9, 2020

I've submitted a PR: flexible-collision-library/fcl#461
As is, it will be rejected, because it bumps CMake minimum version to 3.7. Could be modified a little bit with a less elegant solution in order to preserve CMake version.

Great job!
Why does it need 3.7? The reviewers are talking about supporting 3.5, which is not much less.

In a similar situation, I had to avoid using link_options/target_link_options, and use CMAKE_EXE_LINKER_FLAGS/CMAKE_SHARED_LINKER_FLAGS and similar.
I always added a FIXME about minimum required cmake version.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Apr 9, 2020

Because I use an option of generate_export_header introduced in 3.7 (CUSTOM_CONTENT_FROM_VARIABLE ) in order to inject a bunch of macros (those related to explicit instantiation declaration and definition) at the end of export file:
https://cmake.org/cmake/help/v3.7/module/GenerateExportHeader.html

https://github.com/flexible-collision-library/fcl/blob/3b3a1245d9a4fd50f663e144a5909000db84a2c3/include/fcl/CMakeLists.txt#L39

fmt has similars macros, but fmt doesn't use at all generate_export_header, while fcl does.

@conan-center-bot
Copy link
Collaborator

All green in build 5 (ea8c956bab1a0c84536a2b906f3830ee6fc232d0)! 😊

@danimtb danimtb merged commit 74236b1 into conan-io:master Apr 16, 2020
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.

6 participants