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

[pcl] Move VTK to an optional feature #10449

Merged
merged 6 commits into from
Sep 7, 2020

Conversation

here-mikelley
Copy link
Contributor

Describe the pull request

  • What does your PR fix? Fixes issue #

This doesn't fix an issue.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

All triplets should be supported. Making VTK optional also allows pcl to be compiled as a dynamic library under Linux.

Yes

@msftclas
Copy link

msftclas commented Mar 18, 2020

CLA assistant check
All CLA requirements met.

@NancyLi1013 NancyLi1013 self-assigned this Mar 18, 2020
ports/pcl/CONTROL Outdated Show resolved Hide resolved
ports/pcl/portfile.cmake Outdated Show resolved Hide resolved
ports/pcl/CONTROL Show resolved Hide resolved
@JackBoosY
Copy link
Contributor

/azp run

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Mar 20, 2020
@JackBoosY JackBoosY marked this pull request as ready for review March 20, 2020 06:38
@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label Mar 20, 2020
@JackBoosY
Copy link
Contributor

@here-mikelley Does this issue ready to review?

@here-mikelley
Copy link
Contributor Author

@JackBoosY I believe it is. Please review.

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed waiting for response labels Mar 22, 2020
@dan-shaw
Copy link
Contributor

@here-mikelley Why does VTK need to be an optional feature? You can install pcl without VTK using vcpkg install pcl[core]

@dan-shaw dan-shaw added waiting for response and removed info:reviewed Pull Request changes follow basic guidelines labels Mar 28, 2020
@here-mikelley
Copy link
Contributor Author

here-mikelley commented Mar 30, 2020

@here-mikelley Why does VTK need to be an optional feature? You can install pcl without VTK using vcpkg install pcl[core]

I must be misunderstanding Build-Depends and feature lists. Isn't Build-Depends a list of required dependencies? I thought that the feature list disabled default features and enabled optional features. I interpret pcl[core] to mean that all build dependencies specified in Build-Depends are required and built.

@here-mikelley
Copy link
Contributor Author

@dan-shaw , I just tried your suggestion and it still installs vtk:

PS /Users/mikelley/vcpkg> ./vcpkg/vcpkg install pcl[core]:x64-osx
Computing installation plan...
The following packages will be built and installed:
  * boost-asio[core]:x64-osx
  * boost-coroutine[core]:x64-osx
  * boost-signals2[core]:x64-osx
  * flann[core]:x64-osx
  * openssl[core]:x64-osx
    pcl[core]:x64-osx
  * vtk[core]:x64-osx
Additional packages (*) will be modified to complete this operation.

@NancyLi1013
Copy link
Contributor

@here-mikelley
Could you please resolve the conflicts?

@here-mikelley
Copy link
Contributor Author

@here-mikelley
Could you please resolve the conflicts?

It looks as though VTK was forced to be OFF in the latest code on master. My commit moved VTK to a feature that can be turned off/on, so I'm not sure how to resolve this conflict. We should revisit this change once VTK is enabled again.

ports/pcl/CONTROL Outdated Show resolved Hide resolved
@JackBoosY JackBoosY added requires:testing Needs tests added before merging and removed waiting for response labels May 18, 2020
@NancyLi1013
Copy link
Contributor

Hi @here-mikelley

Feature vtk build failed on x86-windows:

F:\10449\vcpkg\buildtrees\pcl\src\pcl-1.9.1-f2ec88a858\io\src\vtk_lib_io.cpp(369): error C2664: 'int vtkCellArray::GetNextCell(vtkIdList *)': cannot convert argument 2 from 'vtkIdType *' to 'const vtkIdType *&'

Could you please try to look into it?

@NancyLi1013 NancyLi1013 added waiting for response category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels May 19, 2020
@NancyLi1013 NancyLi1013 removed the requires:testing Needs tests added before merging label Jun 3, 2020
@NancyLi1013
Copy link
Contributor

Hi @here-mikelley
Is work still being done for this PR?

@here-mikelley
Copy link
Contributor Author

here-mikelley commented Jun 18, 2020

VTK is currently disabled for pcl as shown below:

-DWITH_VTK=OFF # disabled due to API changes in 9.0

I've paused work on this until it's enabled again.

@NancyLi1013
Copy link
Contributor

Sorry for the late reply.
Seems you have removed this line in this PR.

-DWITH_VTK=OFF # disabled due to API changes in 9.0

@BillyONeal
Copy link
Member

Manually tested:

  • pcl[core]:x86-windows (also checked that it did not install vtk)
  • pcl[core,vtk]:x86-windows
  • pcl[core]:x86-windows-static
  • pcl[core,vtk]:x86-windows-static
  • pcl[core]:x64-osx (also checked that it did not install vtk)
  • pcl[core,vtk]:x64-osx

@BillyONeal BillyONeal added the info:reviewed Pull Request changes follow basic guidelines label Sep 5, 2020
@BillyONeal BillyONeal merged commit 50d5b31 into microsoft:master Sep 7, 2020
@BillyONeal
Copy link
Member

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants