Skip to content

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
Contributor

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

@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
Contributor

All green in build 3 (c3875eb257e069d95a933748982034b6ef207ffe)! 😊

@conan-center-bot
Copy link
Contributor

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.

@SpaceIm SpaceIm dismissed stale reviews from uilianries and SSE4 via ea8c956 April 13, 2020 15:29
@SpaceIm SpaceIm requested review from SSE4 and uilianries April 13, 2020 15:29
@conan-center-bot
Copy link
Contributor

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