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 All-in-one Installer: add process to add/remove VTK path #3322

Merged
merged 2 commits into from
Sep 13, 2019

Conversation

UnaNancyOwen
Copy link
Member

This pull request has two changes.

  • Fix uninstall process to remove environment variable.
    The variable name contained an unnecessary underscore.
    The conditional expression is always false and always runing process to remove variable.
    (It has not been a problem so far, because the remove path process that are not included in environment variables as there is virtually no change.)

    - StrCmp $DO_NOT_ADD_TO_PATH_ "1" doNotRemoveFromPath 0
    + StrCmp $DO_NOT_ADD_TO_PATH "1" doNotRemoveFromPath 0
  • Add install and uninstall process to add and remove VTK path to environment variables PATH.
    This installer change for a temporary solution (It can be avoided by changing to dynamic linked VTK) for issue PCLVisualizer tutorial issue on Windows #1601. It just add/remove the VTK path in addition to the PCL path.
    I have already tested this installer/uninstaller on my environment. It works correctly without problem which option choose. It means we are ready to generate and publish the PCL 1.10.0 All-In-One Installer (that include pre-built PCL with dynamic linked VTK as a temporary solution for this issue).

    C:\Program Files\PCL\bin
    C:\Program Files\PCL\3rdParty\VTK\bin
    

    Note: This change should be undone once the issue PCLVisualizer tutorial issue on Windows #1601 is resolved.

Fix unistall processs to remove environment variable from PATH by remove unnecessary underscores.
Add install and uninstall process to add and remove VTK path to environment variables PATH.
This is temporary solution for issue PointCloudLibrary#1601.
@UnaNancyOwen UnaNancyOwen requested a review from taketwo September 3, 2019 12:58
@UnaNancyOwen UnaNancyOwen added this to the pcl-1.10.0 milestone Sep 3, 2019
@taketwo
Copy link
Member

taketwo commented Sep 3, 2019

Thanks Tsukasa. One thing I don't understand entirely. Who (and how) controls whether the VTK in the all-in-one installer is dynamically or statically linked?

@UnaNancyOwen
Copy link
Member Author

UnaNancyOwen commented Sep 3, 2019

Who (and how) controls whether the VTK in the all-in-one installer is dynamically or statically linked?

We and users don’t have to do anything about it.
For example, even if VTK that included in PCL All-in-one Installer is a static link library, there is nothing pain with adding the path of PCL/3rdParty/VTK/bin to the environment variable (although it is unnecessary). So we don't need to control it.

The VTK configuration that should be included in the PCL All-in-one Installer depends on whether #1601 is resolved at the time of release.
If it is not resolved yet, I will dynamic linking VTK as temporary solution. If it was resolved at release time, I will statically linking VTK in the same way as so far.

I will be controls it and make a PCL All-in-one Installer. (It will made by me in recent years. Probably, It is only me will create PCL All-in-one Installer.)

Even it would be either, users will be able to use the same CMakeLists.txt that you used previously to generate own project. It is handled properly by CMake.

Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification.

@taketwo taketwo merged commit 8e3a2f6 into PointCloudLibrary:master Sep 13, 2019
@taketwo taketwo changed the title Add process to add/remove VTK path in PCL All-in-one Installer PCL All-in-one Installer: add process to add/remove VTK path Jan 14, 2020
@UnaNancyOwen UnaNancyOwen deleted the fix_nsis branch May 12, 2020 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants