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.

Sign up for GitHub

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

Jump to bottom

Unifies Find scripts in PCLConfig #1421

Merged
merged 1 commit into from
Nov 3, 2017
Merged

Unifies Find scripts in PCLConfig #1421

merged 1 commit into from
Nov 3, 2017

Conversation

fran6co
Copy link
Contributor

@fran6co fran6co commented Nov 12, 2015

Supersedes #1416

@jspricke
Copy link
Member

Looks really awesome. I can't test it at the moment, but otherwise +1 :).

@fran6co
Copy link
Contributor Author

fran6co commented Nov 13, 2015

@ahundt could you check if this pr solves your problem?

@jspricke
Copy link
Member

I've been talking to a friend (@v4hn) about it and he proposed to do it rather like this:
v4hn/Las-Vegas-Reconstruction@787b6f7

@v4hn
Copy link
Contributor

v4hn commented Nov 13, 2015

The main idea here is to provide Find-scripts with the library, which are not provided by cmake or the specific dependencies (yet).
In case at some point a Find script becomes available upstream (e.g. FindEigen3.cmake which is provided starting with cmake 3.0) the only thing required to switch to the upstream version is to remove the Find script from the libraries build - no need to touch libConfig.cmake at all.
Also the generated libConfig.cmake probably looks a bit better...

@taketwo
Copy link
Member

taketwo commented Nov 13, 2015

The idea of eliminating redundancy is appealing. However, I have serious concerns about the proposed implementation. There is always a difference between the CMake code in Find***.cmake script and corresponding macro in PCLConfig.cmake. As a result, the diff between the config script generated with current master and with your PR is quite substantial. What increases my concern is that in many cases I just don't know why the scripts are different. Mistake in copy-paste? Sloppy coding? Or a workaround for an issue on some platform?

Just to give an example. The code for detection of DepthSense SDK has slight differences between the "find" script and the "config" script. Originally when I contributed it, it was the same, but then someone showed up (actually @v4hn) and pointed that he gets configuration errors. So we introduced changes in #1337.

With such cases in mind, applying your changes feels like opening a can of worms for me. Especially before 1.8.0 release.

@v4hn
Copy link
Contributor

v4hn commented Nov 13, 2015

On Fri, Nov 13, 2015 at 10:00:09AM -0800, Sergey Alexandrov wrote:

The idea of eliminating redundancy is appealing. However, I have serious concerns about the proposed implementation. There is always a difference between the CMake code in Find***.cmake script and corresponding macro in PCLConfig.cmake. As a result, the diff between the config script generated with current master and with your PR is quite substantial. What increases my concern is that in many cases I just don't know why the scripts are different. Mistake in copy-paste? Sloppy coding? Or a workaround for an issue on some platform?

There are differences, yes. But there shouldn't be any.
If it's a workaround for some specific platform, this workaround should be in both parts of the find logic.

Just to give an example [...] in #1337.

I never noticed the issue number :)

Concerning this particular issue: actually they were not the same before that commit.
the logic in cmake/Modules/FindDSSDK.cmake does not set DSSDK_INCLUDE_DIRS if the module is not found.
The logic in PCLConfig.cmake did this though.
So the bug would not even have appeared without the "duplication". :)

With such cases in mind, applying your changes feels like opening a can of worms for me. Especially before 1.8.0 release.

imho this is a can of worms that should be opened and cleaned up before the next major release.
As there will be a release candidate for the major release,
this is a great opportunity to make sure the resulting "clean" logic is correct for all relevant platforms.

@taketwo
Copy link
Member

taketwo commented Nov 13, 2015

You are probably right about how the things should be. And that this "can" should be opened. However, I would strongly disagree that now is the right time. Granted, we will introduce configuration issues on some systems. What should be the time frame for waiting for people to catch and report them? How much should we postpone the release?

I'd rather release first and then venture into letting the worms out.

@jspricke
Copy link
Member

  • Sergey Alexandrov notifications@github.com [2015-11-13 10:36]:

    I'd rather release first and then venture into letting the worms out.

+1

@fran6co
Copy link
Contributor Author

fran6co commented Nov 13, 2015

In that case maybe it's better to merge #1416, release and then merge this PR. In any case I'm going to change it to @v4hn approach.

@fran6co fran6co force-pushed the pclconfig branch 4 times, most recently from f0e7572 to cc9849a Compare March 9, 2016 02:20
@fran6co
Copy link
Contributor Author

fran6co commented Mar 9, 2016

Rebased and using @v4hn approach. Should be a bit cleaner.

@fran6co
Copy link
Contributor Author

fran6co commented Mar 9, 2016

Would be nice to get rid of the find_ macros and simplify find_external_library a bit, but I think that would need a bit more work.

@SergioRAgostinho SergioRAgostinho added needs: author reply Specify why not closed/merged yet needs: testing Specify why not closed/merged yet labels Aug 22, 2016
@SergioRAgostinho SergioRAgostinho added this to the pcl-1.9.0 milestone Aug 22, 2016
@SergioRAgostinho
Copy link
Member

Hey @fran6co. Can you address the merge conflicts once we release 1.8.1? It would be good to integrate this in the early stage of our preparation for 1.9.0.

SergioRAgostinho
SergioRAgostinho reviewed Aug 22, 2016
View reviewed changes
PCLConfig.cmake.in Outdated
@@ -896,8 +557,6 @@ endif(NOT "${PCL_DEFINITIONS}" STREQUAL "")

pcl_remove_duplicate_libraries(PCL_LIBRARIES PCL_DEDUP_LIBRARIES)
set(PCL_LIBRARIES ${PCL_DEDUP_LIBRARIES})
# Add 3rd party libraries, as user code might include our .HPP implementations
list(APPEND PCL_LIBRARIES ${BOOST_LIBRARIES} ${QHULL_LIBRARIES} ${OPENNI_LIBRARIES} ${OPENNI2_LIBRARIES} ${ENSENSO_LIBRARIES} ${davidSDK_LIBRARIES} ${DSSDK_LIBRARIES} ${RSSDK_LIBRARIES} ${FLANN_LIBRARIES} ${VTK_LIBRARIES})
Copy link
Member

Choose a reason for hiding this comment

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

Is this not necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't quite remember why I removed it, I'm putting it back just in case.

@fran6co
Copy link
Contributor Author

fran6co commented Aug 22, 2016

@SergioRAgostinho I'll rebase it during this week.

@SergioRAgostinho
Copy link
Member

If possible I would wait 'till the actual 1.8.1 release. In theory there's only one more PR in 1.8.1 which touches CMake related things and it should not generate a conflict with this one, but just wanting to prevent the extra effort.

Don't forget about my comment/question in line 900.

@fran6co
Copy link
Contributor Author

fran6co commented Aug 22, 2016

@SergioRAgostinho rebased.

@SergioRAgostinho SergioRAgostinho removed the needs: author reply Specify why not closed/merged yet label Aug 22, 2016
@SergioRAgostinho
Copy link
Member

👍 To be merged once the probation period for 1.8.1 is over.

@SergioRAgostinho
Copy link
Member

@fran6co Can you rebase things again? I'm gonna merge it and cross my fingers :)

@ahundt
Copy link

ahundt commented Nov 1, 2017

without any testing I looked at the changes and it appears reasonable to me. For some reason I lost track of this.

@fran6co
Copy link
Contributor Author

fran6co commented Nov 3, 2017

@SergioRAgostinho done

@SergioRAgostinho SergioRAgostinho merged commit 35f9070 into PointCloudLibrary:master Nov 3, 2017
@SergioRAgostinho
Copy link
Member

Pandora's box opened 😅

@SergioRAgostinho SergioRAgostinho removed the needs: testing Specify why not closed/merged yet label Nov 3, 2017
@ahundt
Copy link

ahundt commented Nov 3, 2017 •
edited
Loading

hey only 2 years to get it merged. :-) at least it is done! Hopefully it is not still 2 years until the next release!

@fran6co fran6co deleted the pclconfig branch November 4, 2017 15:31
@UnaNancyOwen UnaNancyOwen mentioned this pull request Nov 6, 2017
Merged
@SergioRAgostinho SergioRAgostinho mentioned this pull request Nov 7, 2017
Closed
@Robzz Robzz mentioned this pull request Feb 13, 2018
Merged
@SergioRAgostinho SergioRAgostinho mentioned this pull request Aug 6, 2018
Closed
@SergioRAgostinho SergioRAgostinho mentioned this pull request Aug 25, 2018
Merged
@shrijitsingh99 shrijitsingh99 mentioned this pull request Mar 12, 2020
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
module: cmake
Projects
None yet
Milestone
pcl-1.9.0
Development

Successfully merging this pull request may close these issues.
6 participants
@fran6co @jspricke @v4hn @taketwo @SergioRAgostinho @ahundt