SlicerRT - Add extension about extensionsindex HOT 10 CLOSED

Just tried to do an experimental and building the extension fails with the following error:

[...]
-- Setting MIDAS_PACKAGE_URL ...................: http://slicer.kitware.com/midas3/api
-- Setting MIDAS_PACKAGE_EMAIL .................: [email protected]
-- Setting MIDAS_PACKAGE_API_KEY ...............: B7FZbXp2h9oZKuwl2GeWcooZx0ltnu2OsxEtkUms
-- Setting EXTENSION_HOMEPAGE ..................: https://www.assembla.com/spaces/slicerrt
-- Setting EXTENSION_CATEGORY ..................: Radiotherapy
-- Setting EXTENSION_ICONURL ...................: http://wiki.slicer.org/slicerWiki/images/f/f2/SlicerRtExtensionLogo.png
-- Setting EXTENSION_CONTRIBUTORS ..............: Csaba Pinter (PerkLab, Queen's University), Andras Lasso (PerkLab, Queen' [...]
-- Setting EXTENSION_DESCRIPTION ...............: Extensions for radiotherapy research (DICOM-RT import, dose volume histog [...]
-- Setting EXTENSION_SCREENSHOTURLS ............: https://www.assembla.com/spaces/slicerrt/documents/bhB--4vSSr4BSNacwqjQWU [...]
-- Setting EXTENSION_DEPENDS ...................: NA
-- Setting EXTENSION_LICENSE_FILE ..............: /home/jchris/Projects/Slicer4/License.txt
-- Setting EXTENSION_README_FILE ...............: /home/jchris/Projects/Slicer4/README.txt
-- Found Git: /usr/bin/git 
-- Found Subversion: /usr/bin/svn 
-- Configuring Qt loadable module: DicomRtImport [qSlicerDicomRtImportModuleExport.h]
-- Configuring Qt loadable module: DoseAccumulation [qSlicerDoseAccumulationModuleExport.h]
-- Configuring Qt loadable module: DoseVolumeHistogram [qSlicerDoseVolumeHistogramModuleExport.h]
CMake Error at CMakeLists.txt:27 (add_subdirectory):
  add_subdirectory given source "IsoDose" which is not an existing directory.

See http://slicer.cdash.org/viewConfigure.php?buildid=29608

from extensionsindex.

Thanks for testing it. There was a case error in the directory name, fixed now (in r196).

from extensionsindex.

There are now errors specific to unix-like system: http://slicer.cdash.org/viewBuildError.php?buildid=29623

/.../SlicerRT/DoseVolumeHistogram/Logic/vtkSlicerDoseVolumeHistogramLogic.h:56: error: ‘stricmp’ was not declared in this scope
/.../SlicerRT/DoseVolumeHistogram/Logic/vtkSlicerDoseVolumeHistogramLogic.cxx: In member function ‘void vtkSlicerDoseVolumeHistogramLogic::RefreshDvhDoubleArrayNodesFromScene()’:

For reference, here is the email I sent few months ago:

Email of May 29 2012

Everything looks fine within the different CMakeLists.

Few remarks, for sake of consistency:

1. Could you move the Data folder outside of the Testing folder ?
2. Could you rename the variable has it's done here: http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=20260

Trying to build on Ubuntu (10.04, g++ 4.4.3), I got the following errors / warnings:

Remark 1

1. /home/jchris/Projects/DoseVolumeHistogram/Logic/vtkSlicerDoseVolumeHistogramLogic.cxx:161: error: ‘stricmp’ was not declared in this scope

...

If you are doing some case insensitive comparison, may be you could just specify your identifiers (DVH_TYPE_ATTRIBUTE_VALUE, ...) in upper case and always convert to upper case before comparing to it. That way you could use "strcmp"

Remark 2

2. Warnings in qSlicerDoseVolumeHistogramModule.ui

Generating ui_qSlicerDoseVolumeHistogramModule.h
/home/jchris/Projects/DoseVolumeHistogram/Resources/UI/qSlicerDoseVolumeHistogramModule.ui: Warning: Z-order assignment: '' is not a valid widget.
/home/jchris/Projects/DoseVolumeHistogram/Resources/UI/qSlicerDoseVolumeHistogramModule.ui: Warning: Z-order assignment: '' is not a valid widget.

Remark 3

3. line 59 of qSlicerDoseVolumeHistogramModuleWidget.cxx, you are using std::map and std::pair to keep track of QCheckBox and other information (see ChartCheckboxToStructureSetNameMap). You could probably use a QSignalMapper instead. Using QHash, QPair etc .. could also make manipulation easier. I would recommend not to mix style .. Qt container with Qt object. You could then leverage the "foreach" macro that would simplify your code.

When using template, having ">>" is incorrect with gcc ... you should add space and write "> >"

/home/jchris/Projects/DoseVolumeHistogram/qSlicerDoseVolumeHistogramModuleWidget.cxx:59: warning: ‘>>’ operator will be treated as two right angle brackets in C++0x [-Wc++0x-compat]
/home/jchris/Projects/DoseVolumeHistogram/qSlicerDoseVolumeHistogramModuleWidget.cxx:59: warning: suggest parentheses around ‘>>’ expression [-Wc++0x-compat]

Remark 4

4. qSlicerDoseVolumeHistogramModuleWidget.cxx - line 814:

There is no need to dynamically allocate QString !

QString are using implicit sharing under the hood and you should not manage memory yourself. It makes the code more complex and introduce potential leaks.

See http://doc.qt.nokia.com/4.7-snapshot/implicit-sharing.html

Remark 5

5. In Qt code, you probably leverage function like QString::compare passing the Qt::CaseSensitivity

See line 200 of qSlicerDoseVolumeHistogramModuleWidget.cxx - It would then prevent the usage of non portable function like "stricmp"

After fixing this issue, I was able to compile and run the test ... see details below.

That said, I am not sure to understand the problem. Assuming I can the test using ctest .. the extension build system should be able to do to.

$ svn checkout https://subversion.assembla.com/svn/slicerrt/trunk/SlicerRt/src/DoseVolumeHistogram/
$ mkdir DoseVolumeHistogram-build
$ cd DoseVolumeHistogram-build
$ cmake -DSlicer_DIR:PATH=/home/jchris/Projects/Slicer4-Superbuild-Debug/Slicer-build/ ../DoseVolumeHistogram
$ make -j4
// Fix error reported above
$ make -j4
$ ctest -N
Test project /home/jchris/Projects/DoseVolumeHistogram-build
  Test #1: qSlicerDoseVolumeHistogramModuleGenericTest
  Test #2: qSlicerDoseVolumeHistogramModuleWidgetGenericTest
  Test #3: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate
  Test #4: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate_CompareMetrics
  Test #5: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate_CERR
  Test #6: vtkSlicerDoseVolumeHistogramLogicTest_EclipseEnt_CERR
  Test #7: vtkSlicerDoseVolumeHistogramLogicTest_EclipseBreast_CERR
$ ctest -R
Test project /home/jchris/Projects/DoseVolumeHistogram-build
    Start 1: qSlicerDoseVolumeHistogramModuleGenericTest
1/7 Test #1: qSlicerDoseVolumeHistogramModuleGenericTest ............................   Passed    7.46 sec
    Start 2: qSlicerDoseVolumeHistogramModuleWidgetGenericTest
2/7 Test #2: qSlicerDoseVolumeHistogramModuleWidgetGenericTest ......................***Failed    2.86 sec
    Start 3: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate
3/7 Test #3: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate ..................   Passed   12.34 sec
    Start 4: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate_CompareMetrics
4/7 Test #4: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate_CompareMetrics ...***Failed    0.00 sec
    Start 5: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate_CERR
5/7 Test #5: vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate_CERR .............   Passed   11.67 sec
    Start 6: vtkSlicerDoseVolumeHistogramLogicTest_EclipseEnt_CERR
6/7 Test #6: vtkSlicerDoseVolumeHistogramLogicTest_EclipseEnt_CERR ..................***Failed  Error regular expression found in output. Regex=[Warning] 14.28 sec
    Start 7: vtkSlicerDoseVolumeHistogramLogicTest_EclipseBreast_CERR
7/7 Test #7: vtkSlicerDoseVolumeHistogramLogicTest_EclipseBreast_CERR ...............***Failed  Error regular expression found in output. Regex=[Warning] 10.14 sec

43% tests passed, 4 tests failed out of 7

Label Time Summary:
qSlicerDoseVolumeHistogramModule    =  10.32 sec

Total Test time (real) =  58.76 sec

The following tests FAILED:
      2 - qSlicerDoseVolumeHistogramModuleWidgetGenericTest (Failed)
      4 - vtkSlicerDoseVolumeHistogramLogicTest_EclipseProstate_CompareMetrics (Failed)
      6 - vtkSlicerDoseVolumeHistogramLogicTest_EclipseEnt_CERR (Failed)
      7 - vtkSlicerDoseVolumeHistogramLogicTest_EclipseBreast_CERR (Failed)
Errors while running CTest

Hth
Jc

from extensionsindex.

Thanks for the further testing. I was not aware that your earlier findings have not yet been fixed. I've fixed most of them (r198) and entered a ticket for a few remaining ones. See details below.

A Slicer build on linux is in progress but it will take a few more hours, so I could not test the changes on linux. If you have time it would be great if you could start another linux build for the extension and let me know about potential remaining errors.

Thanks a lot in advance.
Andras

1. Could you move the Data folder outside of the Testing folder ?
   => We'll do a reorganization of data folders later (https://www.assembla.com/spaces/slicerrt/tickets/107-fix-jc-s-code-review-findings)

2. Could you rename the variable has it's done here: http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=20260
   => Could you please clarify this?

Remark 1
=> fixed

Remark 2
=> could not reproduce on Windows; Linux build is in progress (https://www.assembla.com/spaces/slicerrt/tickets/107-fix-jc-s-code-review-findings)

Remark 3
=> space between > > fixed
=> not using STL in QT classes - will review later (https://www.assembla.com/spaces/slicerrt/tickets/107-fix-jc-s-code-review-findings)

Remark 4
=> fixed

Remark 5
=> fixed

from extensionsindex.

One build error left happening on linux - See http://slicer.cdash.org/viewBuildError.php?buildid=29663

/.../SlicerRT/DoseVolumeHistogram/Testing/Cxx/vtkSlicerDoseVolumeHistogramLogicTest1.cxx:161:
 error: ‘stricmp’ was not declared in this scope

from extensionsindex.

To answer your question about the naming of variable and the content of CMakeLists.txt

1. In top level CMakeLists.txt, calling include(SlicerEnableExtensionTesting) is not needed anymore
2. For sake of consistency, variable names, indent, comments .. in the module CMakeLists.txt could be updated to be similar to what has been done here

EDIT: Fixing 1 would be nice, Fixing 2 is definitively optional for now and won't prevent the integration of the extension into the index.

from extensionsindex.

Fixed in r200.

EDIT by @jcfr: It now compiles and all tests passes on Linux. Feel free to re-push the topic with an updated SVN revision. Would be nice if you could remove the extra include(SlicerEnableExtensionTesting). As soon as this is done, I will integrate it into the index and it should be then be built by the factory :)

from extensionsindex.

I squashed the commit of topic 34-add-SlicerRT and integrated it into master. When you will have a chance to address the lower priority stylistic issue discussed above, create an other issue on the extension index tracker, and reference it with a new topic.

from extensionsindex.

The continuous build successfully build and packaged the extension on windows 32bit and macOSX. See http://slicer.cdash.org/index.php?project=Slicer4&display=project&filtercount=1&showfilters=1&field1=buildname/string&compare1=65&value1=20655-SlicerRT-svn200&collapse=0

The linux didn't work because the nightly build of last night failed due to a different issue (See slicer issue http://www.na-mic.org/Bug/view.php?id=1961)

On the other hand, the Windows 64bit should have been triggered. Just created an issue to keep track of it. See http://www.na-mic.org/Bug/view.php?id=2338

from extensionsindex.

1. In top level CMakeLists.txt, calling include(SlicerEnableExtensionTesting) is not needed anymore
2. For sake of consistency, variable names, indent, comments .. in the module CMakeLists.txt could be updated to be similar to what has been done here

Thanks for the suggestions, fixed both.

from extensionsindex.