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

ENH: Simplify reuse of vtkAddon integrating it as a Slicer remote project #4765

Merged
merged 1 commit into from
Mar 25, 2020

Conversation

jcfr
Copy link
Member

@jcfr jcfr commented Mar 24, 2020

This commit partially reverts 67456af (ENH: Add option to build Slicer
using system vtkAddon) and includes vtkAddon as a Slicer remote.

The call to Slicer_Remote_Add defines Slicer_BUILD_vtkAddon and
vtkAddon_SOURCE_DIR CMake variables.

The source of vtkAddon have been extracted and published in a standalone
repository that can be built independently.
See https://github.com/Slicer/vtkAddon

For future reference, source have been extracted using the following
commands:

  # Clone
  git clone git://github.com/Slicer/Slicer /tmp/Slicer-for-vtkAddon

  cd /tmp/Slicer-for-vtkAddon

  # Extract history specific to vtkAddon
  git-rocket-filter --branch vtkAddon \
    --keep Libs/vtkAddon/ \
    --detach

  # Remove empty directories
  find . -depth -empty -not -path '*/\.*' -delete

  # Publish repo
  git remote add vtkAddon [email protected]:Slicer/vtkAddon

  # Clone
  git clone [email protected]:Slicer/vtkAddon /tmp/vtkAddon

  cd /tmp/vtkAddon


  # Move files from 'Libs/vtkAddon' directory to the root
  git filter-branch --force --tree-filter \
      'mv Libs/vtkAddon/* ./ > /dev/null 2>&1 || true' \
      --tag-name-filter cat -- --all

  # Re-publish
  git push origin master --force

@jcfr jcfr requested a review from Sunderlandkyl March 24, 2020 23:02
@jcfr
Copy link
Member Author

jcfr commented Mar 24, 2020

Let's also note that the new repository https://github.com/Slicer/vtkAddon is not a fork of https://github.com/lorensen/vtkAddon.

Instead I was thinking to add an History section to the README of Slicer/vtkAddon to provide some more details

@lassoan @pieper Does that sound reasonable ?

Finally, note that the work done by @lorensen is not lost. Indeed, in few days I will update Slicer/vtkAddon to support being built as a remote module by backporting the relevant commit from @lorensen

Edited: This was done in Slicer/vtkAddon#4

@jcfr
Copy link
Member Author

jcfr commented Mar 24, 2020

There is an error with python wrapping:

vtkWrapHierarchy: couldn't open file /home/jcfr/Projects/Slicer-Qt5-VTK8-Release/Slicer-build/vtkAddonHierarchy.txt
Modules/Loadable/Terminologies/Logic/CMakeFiles/vtkSlicerTerminologiesModuleLogicHierarchy.dir/build.make:70: recipe for target 'vtkSlicerTerminologiesModuleLogicHierarchy.txt' failed
make[2]: *** [vtkSlicerTerminologiesModuleLogicHierarchy.txt] Error 1
CMakeFiles/Makefile2:5530: recipe for target 'Modules/Loadable/Terminologies/Logic/CMakeFiles/vtkSlicerTerminologiesModuleLogicHierarchy.dir/all' failed
make[1]: *** [Modules/Loadable/Terminologies/Logic/CMakeFiles/vtkSlicerTerminologiesModuleLogicHierarchy.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
vtkWrapHierarchy: couldn't open file /home/jcfr/Projects/Slicer-Qt5-VTK8-Release/Slicer-build/vtkAddonHierarchy.txt
Modules/Loadable/Units/Logic/CMakeFiles/vtkSlicerUnitsModuleLogicHierarchy.dir/build.make:64: recipe for target 'vtkSlicerUnitsModuleLogicHierarchy.txt' failed
make[2]: *** [vtkSlicerUnitsModuleLogicHierarchy.txt] Error 1
CMakeFiles/Makefile2:4933: recipe for target 'Modules/Loadable/Units/Logic/CMakeFiles/vtkSlicerUnitsModuleLogicHierarchy.dir/all' failed
make[1]: *** [Modules/Loadable/Units/Logic/CMakeFiles/vtkSlicerUnitsModuleLogicHierarchy.dir/all] Error 2
vtkWrapHierarchy: couldn't open file /home/jcfr/Projects/Slicer-Qt5-VTK8-Release/Slicer-build/vtkAddonHierarchy.txt
Modules/Loadable/Cameras/Logic/CMakeFiles/vtkSlicerCamerasModuleLogicHierarchy.dir/build.make:64: recipe for target 'vtkSlicerCamerasModuleLogicHierarchy.txt' failed
make[2]: *** [vtkSlicerCamerasModuleLogicHierarchy.txt] Error 1
CMakeFiles/Makefile2:4511: recipe for target 'Modules/Loadable/Cameras/Logic/CMakeFiles/vtkSlicerCamerasModuleLogicHierarchy.dir/all' failed
make[1]: *** [Modules/Loadable/Cameras/Logic/CMakeFiles/vtkSlicerCamerasModuleLogicHierarchy.dir/all] Error 2

Updated: This was fixed by ensuring variables vtkAddon_USE_UTF8 and vtkAddon_WRAP_PYTHON are set.

…ject

This commit partially reverts 67456af (ENH: Add option to build Slicer
using system vtkAddon) and includes vtkAddon as a Slicer remote.

The call to Slicer_Remote_Add defines Slicer_BUILD_vtkAddon and
vtkAddon_SOURCE_DIR CMake variables.

The source of vtkAddon have been extracted and published in a standalone
repository that can be built independently.
See https://github.com/Slicer/vtkAddon

For future reference, source have been extracted using the following
commands:

  # Clone
  git clone git://github.com/Slicer/Slicer /tmp/Slicer-for-vtkAddon

  cd /tmp/Slicer-for-vtkAddon

  # Extract history specific to vtkAddon
  git-rocket-filter --branch vtkAddon \
    --keep Libs/vtkAddon/ \
    --detach

  # Remove empty directories
  find . -depth -empty -not -path '*/\.*' -delete

  # Publish repo
  git remote add vtkAddon [email protected]:Slicer/vtkAddon

  # Clone
  git clone [email protected]:Slicer/vtkAddon /tmp/vtkAddon

  cd /tmp/vtkAddon


  # Move files from 'Libs/vtkAddon' directory to the root
  git filter-branch --force --tree-filter \
      'mv Libs/vtkAddon/* ./ > /dev/null 2>&1 || true' \
      --tag-name-filter cat -- --all

  # Re-publish
  git push origin master --force
@jcfr jcfr force-pushed the add-vtkaddon-as-remote-module branch from 44686bc to d9e129d Compare March 24, 2020 23:32
@jcfr jcfr requested a review from lassoan March 25, 2020 01:06
@jcfr
Copy link
Member Author

jcfr commented Mar 25, 2020

Local linux build completed 👍

@Sunderlandkyl
Copy link
Member

@jcfr This will be very useful. Thanks for doing it!

@jcfr
Copy link
Member Author

jcfr commented Mar 25, 2020

Could you approve so that I can merge ?

@jcfr jcfr merged commit aaf75e2 into Slicer:master Mar 25, 2020
@jcfr jcfr deleted the add-vtkaddon-as-remote-module branch March 25, 2020 01:52
@cpinter
Copy link
Member

cpinter commented Mar 25, 2020

Thanks for working on this! Now that we have a process to do this, can we repeat it for vtkSegmentationCore? It is actually used by other projects (MITK, OHIF), but it is so far in the Slicer source tree just like vtkAddon. I created a separate repo for it mainly for publication purposes (https://github.com/PerkLab/PolySeg) but the latest fixes are not integrated in it, so it would be nice if Slicer used vtkSegmentationCore as remote just like what happened now with vtkAddon. Thanks!

@jcfr
Copy link
Member Author

jcfr commented Mar 25, 2020

Thanks for the feedback

can we repeat it for vtkSegmentationCore

Yes. I will extract the history and create the repo.

The advantage of adding the project as a "remote" is that associate test are still executed as part of the Slicer tests.

@pieper
Copy link
Member

pieper commented Mar 25, 2020

This is an interesting path. We could also consider factoring out other parts of slicer and making them available for use in C++ or python.

@jcfr
Copy link
Member Author

jcfr commented Mar 25, 2020

We could also consider factoring out other parts of slicer and making them available for use in C++ or python.

That makes sense, as we move forward with the transition to use the latest VTK, the will probably move into this direction.

@lassoan
Copy link
Contributor

lassoan commented Mar 25, 2020

MRML would be another great candidate.

This would all very impactful if the libraries were made available on PyPI.

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

Successfully merging this pull request may close these issues.

5 participants