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

Teem fix #5

Open
wants to merge 35 commits into
base: master
Choose a base branch
from
Open

Teem fix #5

wants to merge 35 commits into from

Conversation

Chaircrusher
Copy link

This should fix the UKF standalone build

pnlbwh and others added 30 commits June 25, 2014 15:33
bugfix for fibertractdispersion
In a heirarchial build, it is unnecessary to recursively include
the find_package options in subdirectories.
Since calling 'cmake_minimum_required' with version X.Y.Z implies that
policy associated with version X.Y.Z will be set, this commit
removes the redundant call to "cmake_policy".

For more details, see [1]

[1] http://www.cmake.org/cmake/help/v2.8.9/cmake.html#command:cmake_minimum_required
Since the checks are already done in "Common.cmake" that is included
from both "SuperBuild.cmake" and "UKFTractography.cmake", this commit
removed the duplicate code.
Since the variable MODULE_INCLUDE_DIRECTORIES is passed to the macro
SEMMacroBuildCLI, there are no need to manually include the directories.

This commit removes the "redundant" call.
The top-level CMakeLists.cmake, based on the value of the
UKFTractography_SUPERBUILD variable, already conditionally includes either
UKFTractography.cmake or SuperBuild.cmake.

This commit removes the SuperBuild/CMakeLists.txt file that contain
code already existing in SuperBuild.cmake.
Since the project makes use of SEMMacroBuildCLI macro, there is no
need to selectively integrate "GenerateCLP" and then manually include
the macro. By simply looking up the  SlicerExecutionModel package
and including the associated *_USE_FILE, all required macro will be
available.
…rentiated

This commits enables the following cases:

* By default the extension will be built as a standalone project using
superbuild to download the required dependencies.

* Slicer extension can simply be built specifying
   -DUKFTractography_BUILD_SLICER_EXTENSION:BOOL=ON
   -DSlicer_DIR:PATH=/path/to/Slicer-SuperBuild/Slicer-build
   -DEigen_DIR:PATH=/path/to/Eigen-build
Since the teem library provides a TeemConfig.cmake file if it is either
build as part of Superbuild or install on the system, it is sufficient
to use the "find_package()" call.

This commit fixes a configure error [1] by removing the extra call to
"find_library" that ended up finding the teem library installed on the
system.

More specifically, in the Slicer case, the Teem_DIR was used by the
previous call to "find_package(teem)" to find the teem library built by
Slicer.

Modules ended up referencing a mix of system teem library and Slicer teem
library.

[1] Configure rrror fixed by this commit:

8<----8<----8<----8<----8<----8<----8<----8<----
Cannot generate a safe linker search path for target ConvertVTK because
files in some directories may conflict with libraries in implicit
directories:

  link library [libteem.so] in /usr/lib may be hidden by files in:
    /home/jchris/Projects/Slicer-2-SuperBuild-Release/teem-build/bin
8<----8<----8<----8<----8<----8<----8<----8<----
On system without Qt development libraries installed in a default system
location, Eigen couldn't be built.

This commit removed the Qt dependency by disabling the test depending
on Qt.
This commit update the external project so that it conditionally
download and build teem.
This commit remove the Slicer specific settings from SlicerExectionModel
and ensure the project will be configured with the expected values. Then,
it is not needed anymore to overwrite the SlicerExecutionModel_DEFAULT_*
variable in the current scope.

This has been tested for both a SlicerExtension and a standalone built.
In both case the libraries are built where expected.
Instead of manually creating list of option to give to each project, this
commits removed redundant code and leverage the "mark_as_superbuild"
macro.

By specifying the ALL_PROJECTS value, we ensure that the option will be
passed down to all external projects.

By using "ExternalProject_Include_Dependencies", the CMAKE_GENERATOR
option will be automatically passed to each project using it. This commit
removed the explicit management of this parameter.

Finally, since the passing of the _DIR variable is already set by calling
"mark_as_superbuild" in each external project, this commit removed
the explicit management of these.
…onfigure-errors

Simplify buildsystem and fix configure errors
Due to Artichoke bug pnlbwh#6 [1], it is required to set the value in the cache
so that they can be considered when configuring the external projects.

This commit will fix the build error reported below:

// ----
/usr/bin/ld: /projects/pnl/home/petersv/testukf/build/lib/libitksys-4.6.a(SystemTools.cxx.o): relocation R_X86_64_32 against `.rodata.str1.1' can not be used when making a shared object; recompile with -fPIC
/projects/pnl/home/petersv/testukf/build/lib/libitksys-4.6.a: could not read symbols: Bad value
collect2: ld returned 1 exit status
// ----

[1] commontk/Artichoke#6
COMP: Ensure CMAKE flags are passed down to all external project.
Fixing some bugs in computedispersion.cxx that probably resulted from typos.
In addition, I changed the line if(inDiskPlaneCount > 0) to > 10
for speed up. There is no need to perform the computations inside this
if statement if inDiskPlaneCount < 10, as the function will return false
in this case - see the following if statement.
COMP: remove obsolete --simpleTensorModel flag from tests

COMP: fixed path for teem libraries
COMP: Fix vtk fiber bundle writer

This was work done by Kent Williams
…scalar-value-per-point data gets written as FieldData.
BUG: always write out DDF as THE scalar data for a fiber bundleOther sca...
…e XML

Added grant numbers to acknowledgements, added some names to contributors list, and changed some default settings in the UI as suggested by Ron.
Reverting some of the changes to teem library paths made in commit cf25b45, July 11th 2014, which were causing the Slicer Extension build to crash.
Peter Savadjiev and others added 5 commits July 28, 2014 15:58
COMP: Make CompressedSensing and fibertractdisperssion optional

The default is not to build either
The existing code works to build the UKF slicer extension
and the new search code might not work when built as a
slicer extension, so use the old behavior if the
extension is being built.
@petersv2
Copy link

Hi Kent,

I don't have write access to the BRAINSia repositories, I only have
access to the pnlbwh ones, so I cannot merge this pull request.

Peter

On 08/18/2014 02:13 PM, Kent Williams wrote:

This should fix the UKF standalone build


    You can merge this Pull Request by running

git pull https://github.com/Chaircrusher/ukftractography TeemFix

Or view, comment on, or merge it at:

#5

    Commit Summary

* Merge pull request #24 from BRAINSia/master
* ENH: Clean-up build options
* ENH: Make compliant with standard extension layout.
* ENH: Required to have no variables in the EXTENSION_ variables.
* COMP: Remove unneeded call to "cmake_policy"
* COMP: Remove duplicated MacOSX platform checks
* COMP: Remove redundant directory includes in ukf/CMakeLists.txt
* COMP: Remove unused CMakeLists.txt in SuperBuild folder
* COMP: Simplify inclusion of SlicerExecutionModel package
* COMP: Simplify how standalone/superbuild and extension case are
  differentiated
* COMP: Consistently find teem library
* STYLE: Use consistent indentation in all CMakeLists.txt
* STYLE: Remove unused code from Eigen external project
* COMP: Fix build error disabling Eigen Qt dependency
* COMP: Update teem project to allow use of system teem.
* COMP: Simplify management of output and install directories
* COMP: Simplify passing of variable to external projects
* Merge pull request #28 from
  jcfr/simplify-buildsystem-and-fix-configure-errors
* COMP: Ensure CMAKE flags are passed down to all external project.
* Merge pull request #29 from jcfr/fix-relocation-build-error
* BUG: fixing some bugs in computedispersion.cxx
* COMP: Fix vtk fiber bundle writer
* Merge pull request #34 from Chaircrusher/FixVTKPolyDataWriter
* BUG: always write out DDF as THE scalar data for a fiber
  bundleOther scalar-value-per-point data gets written as FieldData.
* Merge pull request #35 from Chaircrusher/FixVTKPolyDataWriter
* Changed acknowledgments, contributors and some default settings
  in the XML
* Fixing Teem library path for the Slicer Extension
* Fixed a typo in previous commit
* Updated list of contributors
* Updated list of contributors #2
* One more change to the UKFTractography XML, suggested by Ron
* COMP: fix linking to teem on linux
* COMP: conditionally search for Teem library
* Merge pull request #37 from Chaircrusher/FixTeemLink
* BUG: inverted test for Slicer build


    File Changes

* *M* CMakeLists.txt
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-0> (29)

* *M* Common.cmake
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-1> (53)

* *M* CompressedSensing/CMakeLists.txt
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-2> (51)

* *M* CompressedSensing/Testing/CMakeLists.txt
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-3> (2)

* *M* SuperBuild.cmake
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-4> (312)

* *D* SuperBuild/CMakeLists.txt
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-5> (129)

* *M* SuperBuild/External_Boost.cmake
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-6> (22)

* *M* SuperBuild/External_Eigen.cmake
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-7> (44)

* *M* SuperBuild/External_JPEG.cmake
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-8> (25)

* *M* SuperBuild/External_OpenJPEG.cmake
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-9> (46)

* *M* SuperBuild/External_SlicerExecutionModel.cmake
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-10>
  (11)
* *M* SuperBuild/External_TIFF.cmake
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-11>
  (25)
* *M* SuperBuild/External_VTK.cmake
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-12>
  (2)
* *M* SuperBuild/External_teem.cmake
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-13>
  (160)
* *M* UKFTractography.cmake
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-14>
  (75)
* *M* common/CMakeLists.txt
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-15>
  (4)
* *M* fibertractdispersion/CMakeLists.txt
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-16>
  (54)
* *M* fibertractdispersion/Testing/CMakeLists.txt
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-17>
  (4)
* *M* fibertractdispersion/computedispersion.cxx
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-18>
  (10)
* *M* fibertractdispersion/fiberbundle.cxx
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-19>
  (10)
* *M* ukf/CMakeLists.txt
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-20>
  (81)
* *M* ukf/Testing/Cxx/CMakeLists.txt
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-21>
  (244)
* *M* ukf/UKFTractography.xml
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-22>
  (10)
* *M* vtk2mask/CMakeLists.txt
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-23>
  (44)
* *D* vtk2mask/Slicer4ExtensionBuild/CMakeLists.txt
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-24>
  (110)
* *M* vtkFilter/CMakeLists.txt
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-25>
  (47)
* *D* vtkFilter/Slicer4ExtensionBuild/CMakeLists.txt
  <https://github.com/BRAINSia/ukftractography/pull/5/files#diff-26>
  (113)


    Patch Links:

* https://github.com/BRAINSia/ukftractography/pull/5.patch
* https://github.com/BRAINSia/ukftractography/pull/5.diff


Reply to this email directly or view it on GitHub
#5.

The information in this e-mail is intended only for the person to whom it is
addressed. If you believe this e-mail was sent to you in error and the e-mail
contains patient information, please contact the Partners Compliance HelpLine at
http://www.partners.org/complianceline . If the e-mail was sent to you in error
but does not contain patient information, please contact the sender and properly
dispose of the e-mail.

@Chaircrusher
Copy link
Author

I didn’t know you needed access to the pulled-from repo to merge a pull request.

Why don’t you just make this change?

index 0ced8c2..ac39e35 100644
--- a/UKFTractography.cmake
+++ b/UKFTractography.cmake
@@ -37,7 +37,7 @@ find_package(ZLIB REQUIRED)
#-----------------------------------------------------------------------------
find_package(Teem REQUIRED)
include(${Teem_USE_FILE})
-if(NOT ${PRIMARY_PROJECT_NAME}_BUILD_SLICER_EXTENSION)
+if(${PRIMARY_PROJECT_NAME}_BUILD_SLICER_EXTENSION)
set(TEEM_LIB teem)
else()

From: petersv2 <[email protected]mailto:[email protected]>
Reply-To: BRAINSia/ukftractography <[email protected]mailto:[email protected]>
Date: Monday, August 18, 2014 at 1:19 PM
To: BRAINSia/ukftractography <[email protected]mailto:[email protected]>
Cc: Mushly McMushmaster <[email protected]mailto:[email protected]>
Subject: Re: [ukftractography] Teem fix (#5)

Hi Kent,

I don't have write access to the BRAINSia repositories, I only have
access to the pnlbwh ones, so I cannot merge this pull request.

Peter

On 08/18/2014 02:13 PM, Kent Williams wrote:

This should fix the UKF standalone build


You can merge this Pull Request by running

git pull https://github.com/Chaircrusher/ukftractography TeemFix

Or view, comment on, or merge it at:

#5

Commit Summary

  • Merge pull request bugfix for fibertractdispersion pnlbwh/ukftractography#24 from BRAINSia/master
  • ENH: Clean-up build options
  • ENH: Make compliant with standard extension layout.
  • ENH: Required to have no variables in the EXTENSION_ variables.
  • COMP: Remove unneeded call to "cmake_policy"
  • COMP: Remove duplicated MacOSX platform checks
  • COMP: Remove redundant directory includes in ukf/CMakeLists.txt
  • COMP: Remove unused CMakeLists.txt in SuperBuild folder
  • COMP: Simplify inclusion of SlicerExecutionModel package
  • COMP: Simplify how standalone/superbuild and extension case are
    differentiated
  • COMP: Consistently find teem library
  • STYLE: Use consistent indentation in all CMakeLists.txt
  • STYLE: Remove unused code from Eigen external project
  • COMP: Fix build error disabling Eigen Qt dependency
  • COMP: Update teem project to allow use of system teem.
  • COMP: Simplify management of output and install directories
  • COMP: Simplify passing of variable to external projects
  • Merge pull request Simplify buildsystem and fix configure errors pnlbwh/ukftractography#28 from
    jcfr/simplify-buildsystem-and-fix-configure-errors
  • COMP: Ensure CMAKE flags are passed down to all external project.
  • Merge pull request COMP: Ensure CMAKE flags are passed down to all external project. pnlbwh/ukftractography#29 from jcfr/fix-relocation-build-error
  • BUG: fixing some bugs in computedispersion.cxx
  • COMP: Fix vtk fiber bundle writer
  • Merge pull request COMP: Fix vtk fiber bundle writer pnlbwh/ukftractography#34 from Chaircrusher/FixVTKPolyDataWriter
  • BUG: always write out DDF as THE scalar data for a fiber
    bundleOther scalar-value-per-point data gets written as FieldData.
  • Merge pull request BUG: always write out DDF as THE scalar data for a fiber bundleOther sca... pnlbwh/ukftractography#35 from Chaircrusher/FixVTKPolyDataWriter
  • Changed acknowledgments, contributors and some default settings
    in the XML
  • Fixing Teem library path for the Slicer Extension
  • Fixed a typo in previous commit
  • Updated list of contributors
  • Updated list of contributors Fix vtk writer #2
  • One more change to the UKFTractography XML, suggested by Ron
  • COMP: fix linking to teem on linux
  • COMP: conditionally search for Teem library
  • Merge pull request Fix teem link pnlbwh/ukftractography#37 from Chaircrusher/FixTeemLink
  • BUG: inverted test for Slicer build

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#5.

The information in this e-mail is intended only for the person to whom it is
addressed. If you believe this e-mail was sent to you in error and the e-mail
contains patient information, please contact the Partners Compliance HelpLine at
http://www.partners.org/complianceline . If the e-mail was sent to you in error
but does not contain patient information, please contact the sender and properly
dispose of the e-mail.


Reply to this email directly or view it on GitHubhttps://github.com//pull/5#issuecomment-52532844.


Notice: This UI Health Care e-mail (including attachments) is covered by the Electronic Communications Privacy Act, 18 U.S.C. 2510-2521, is confidential and may be legally privileged. If you are not the intended recipient, you are hereby notified that any retention, dissemination, distribution, or copying of this communication is strictly prohibited. Please reply to the sender that you have received the message in error, then delete it. Thank you.


@petersv2
Copy link

I didn't know either, but I just tried and it didn't work :-) OK, I'll
just make the change myself then.

Thanks,
Peter

On 08/18/2014 02:23 PM, Kent Williams wrote:

I didn’t know you needed access to the pulled-from repo to merge a
pull request.

Why don’t you just make this change?

index 0ced8c2..ac39e35 100644
--- a/UKFTractography.cmake
+++ b/UKFTractography.cmake
@@ -37,7 +37,7 @@ find_package(ZLIB REQUIRED)
#-----------------------------------------------------------------------------

find_package(Teem REQUIRED)
include(${Teem_USE_FILE})
-if(NOT ${PRIMARY_PROJECT_NAME}_BUILD_SLICER_EXTENSION)
+if(${PRIMARY_PROJECT_NAME}_BUILD_SLICER_EXTENSION)
set(TEEM_LIB teem)
else()

From: petersv2
<[email protected]mailto:[email protected]>
Reply-To: BRAINSia/ukftractography
<[email protected]mailto:[email protected]>
Date: Monday, August 18, 2014 at 1:19 PM
To: BRAINSia/ukftractography
<[email protected]mailto:[email protected]>
Cc: Mushly McMushmaster
<[email protected]mailto:[email protected]>
Subject: Re: [ukftractography] Teem fix (#5)

Hi Kent,

I don't have write access to the BRAINSia repositories, I only have
access to the pnlbwh ones, so I cannot merge this pull request.

Peter

On 08/18/2014 02:13 PM, Kent Williams wrote:

This should fix the UKF standalone build


You can merge this Pull Request by running

git pull https://github.com/Chaircrusher/ukftractography TeemFix

Or view, comment on, or merge it at:

#5

Commit Summary

  • Merge pull request bugfix for fibertractdispersion pnlbwh/ukftractography#24 from BRAINSia/master
  • ENH: Clean-up build options
  • ENH: Make compliant with standard extension layout.
  • ENH: Required to have no variables in the EXTENSION_ variables.
  • COMP: Remove unneeded call to "cmake_policy"
  • COMP: Remove duplicated MacOSX platform checks
  • COMP: Remove redundant directory includes in ukf/CMakeLists.txt
  • COMP: Remove unused CMakeLists.txt in SuperBuild folder
  • COMP: Simplify inclusion of SlicerExecutionModel package
  • COMP: Simplify how standalone/superbuild and extension case are
    differentiated
  • COMP: Consistently find teem library
  • STYLE: Use consistent indentation in all CMakeLists.txt
  • STYLE: Remove unused code from Eigen external project
  • COMP: Fix build error disabling Eigen Qt dependency
  • COMP: Update teem project to allow use of system teem.
  • COMP: Simplify management of output and install directories
  • COMP: Simplify passing of variable to external projects
  • Merge pull request Simplify buildsystem and fix configure errors pnlbwh/ukftractography#28 from
    jcfr/simplify-buildsystem-and-fix-configure-errors
  • COMP: Ensure CMAKE flags are passed down to all external project.
  • Merge pull request COMP: Ensure CMAKE flags are passed down to all external project. pnlbwh/ukftractography#29 from jcfr/fix-relocation-build-error
  • BUG: fixing some bugs in computedispersion.cxx
  • COMP: Fix vtk fiber bundle writer
  • Merge pull request COMP: Fix vtk fiber bundle writer pnlbwh/ukftractography#34 from Chaircrusher/FixVTKPolyDataWriter
  • BUG: always write out DDF as THE scalar data for a fiber
    bundleOther scalar-value-per-point data gets written as FieldData.
  • Merge pull request BUG: always write out DDF as THE scalar data for a fiber bundleOther sca... pnlbwh/ukftractography#35 from Chaircrusher/FixVTKPolyDataWriter
  • Changed acknowledgments, contributors and some default settings
    in the XML
  • Fixing Teem library path for the Slicer Extension
  • Fixed a typo in previous commit
  • Updated list of contributors
  • Updated list of contributors Fix vtk writer #2
  • One more change to the UKFTractography XML, suggested by Ron
  • COMP: fix linking to teem on linux
  • COMP: conditionally search for Teem library
  • Merge pull request Fix teem link pnlbwh/ukftractography#37 from Chaircrusher/FixTeemLink
  • BUG: inverted test for Slicer build

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#5.

The information in this e-mail is intended only for the person to whom
it is
addressed. If you believe this e-mail was sent to you in error and the
e-mail
contains patient information, please contact the Partners Compliance
HelpLine at
http://www.partners.org/complianceline . If the e-mail was sent to you
in error
but does not contain patient information, please contact the sender
and properly
dispose of the e-mail.


Reply to this email directly or view it on
GitHubhttps://github.com//pull/5#issuecomment-52532844.


Notice: This UI Health Care e-mail (including attachments) is covered
by the Electronic Communications Privacy Act, 18 U.S.C. 2510-2521, is
confidential and may be legally privileged. If you are not the
intended recipient, you are hereby notified that any retention,
dissemination, distribution, or copying of this communication is
strictly prohibited. Please reply to the sender that you have received
the message in error, then delete it. Thank you.



Reply to this email directly or view it on GitHub
#5 (comment).

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

Successfully merging this pull request may close these issues.

4 participants