Skip to content

Conversation

@Ram-Z
Copy link

@Ram-Z Ram-Z commented Apr 23, 2019

This is the first part of #2967, it will only look for the components of VTK that are actually used by PCL.

@taketwo taketwo added needs: code review Specify why not closed/merged yet module: cmake labels Apr 23, 2019
@taketwo
Copy link
Member

taketwo commented Apr 24, 2019

I haven't had time to review in detail yet, but from a quick look through I noticed that this will break QVTK detection. This line calls this script. It does not really find QVTK, but rather checks if the necessary components were found before. This won't work anymore because we don't search for these components.

@Ram-Z
Copy link
Author

Ram-Z commented Apr 24, 2019

Ah yes... I should probably approach this with a bit more rigor now, rather than the quick and dirty stuff I was doing in the other PR.

The way I've been testing this is by building with an Ubuntu Xenial docker image build from this
Dockerfile and my own system running Arch and having random stuff installed.

How would I go about replicating your buildsystem?

@taketwo
Copy link
Member

taketwo commented Apr 24, 2019

We use this image on Azure CI. It was generated with this Dockerfile.

@taketwo
Copy link
Member

taketwo commented Apr 25, 2019

One more thing to keep in mind: there are two distinct points in time when VTK is looked up. The first is during library configuration time; this PR takes care of it. The second is when a downstream project looks up PCL. At this moment PCLConfig file is processed and all third-party dependencies of PCL are "re-discovered". As it is now, PCLConfig still searches for the entirety of VTK, thus downstream projects linking against PCL will still link against every VTK module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: cmake needs: code review Specify why not closed/merged yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants