QIICR / dcmqi

dcmqi (DICOM for Quantitative Imaging) is a free, open source C++ library for conversion between imaging research formats and the standard DICOM representation for image analysis results
https://qiicr.gitbook.io/dcmqi-guide/
BSD 3-Clause "New" or "Revised" License
241 stars 61 forks source link

Building dcmqi using non-superbuild approach #367

Closed yurivict closed 5 years ago

yurivict commented 5 years ago

It fails:

CMake Error at apps/paramaps/CMakeLists.txt:15 (find_package):
  By not providing "FindSlicerExecutionModel.cmake" in CMAKE_MODULE_PATH this
  project has asked CMake to find a package configuration file provided by
  "SlicerExecutionModel", but CMake did not find one.

  Could not find a package configuration file provided by
  "SlicerExecutionModel" with any of the following names:

    SlicerExecutionModelConfig.cmake
    slicerexecutionmodel-config.cmake

Version 1.2.0

yurivict commented 5 years ago

Slicer doesn't even install SlicerExecutionModelConfig.cmake, see here https://github.com/Slicer/SlicerExecutionModel/issues/106

fedorov commented 5 years ago

@yurivict can you provide more details about what you are trying to do and how? Can you add step by step what you are doing?

DCMQI_BUILD_SLICER_EXTENSION is set to OFF by default. If you want to build dcmqi without Slicer, you do not need to change any option. dcmqi is using SlicerExecutionModel, but that will be checked out / configured / built as part of the superbuild process.

fedorov commented 5 years ago

For the sake of completeness, build instructions are available here in the user guide: https://qiicr.gitbooks.io/dcmqi-guide/user_guide/build_dcmqi.html#superbuild-approach

yurivict commented 5 years ago

I run cmake -DDCMQI_SUPERBUILD=OFF -DDCMQI_BUILD_SLICER_EXTENSION=OFF -DBUILD_TESTING=OFF ., and it fails with the above message.

Downloading packages on the fly is not desirable because

  1. this assumes that they are easy to build which they are usually not (this is maybe only easy on Linux)
  2. it is insecure to download code without checking hash of the downloaded files
yurivict commented 5 years ago

You have these lines:

#
# SlicerExecutionModel
#
find_package(SlicerExecutionModel REQUIRED)
include(${SlicerExecutionModel_USE_FILE})

They aren't conditional on DCMQI_BUILD_SLICER_EXTENSION, so it always looks for the Slicer.

fedorov commented 5 years ago

Thank you for providing these details. Indeed, you cannot disable superbuild and disable Slicer extension at the same time. It should be possible to build all dependencies separately, and then point cmake to those, but I have not tried that, and I don't know if it will work. The command you specified will not work for sure though, since you would need to point the build to the dependencies.

this assumes that they are easy to build which they are usually not (this is maybe only easy on Linux)

We have continuous integration for Windows, Mac and Linux, so this gives us some confidence it is working. At the moment, all 3 are green.

it is insecure to download code without checking hash of the downloaded files

How do you download code from GitHub? The superbuild process uses git clone from specific locations that you can see for the individual dependencies here: https://github.com/QIICR/dcmqi/tree/master/CMakeExternals.

If you are that cautious, I think you are already assume significant risks by simply taking the source code from repositories, since you can't really tell what the code inside of those is doing exactly. You have to trust something at some point.

They aren't conditional on DCMQI_BUILD_SLICER_EXTENSION, so it always looks for the Slicer.

No, it is not looking for Slicer, it is looking for SlicerExecutionModel. Those are very different. SlicerExecutionModel is a very lightweight library that helps with wrapping of command line tools and adding SlicerExecutionModel interface.

yurivict commented 5 years ago

How do you download code from GitHub?

I download the tarball of the project from GitHub, as this is done for all other GitHub projects.

I already have all packages listed in https://github.com/QIICR/dcmqi/tree/master/CMakeExternals.

If you are that cautious, I think you are already assume significant risks by simply taking the source code from repositories, since you can't really tell what the code inside of those is doing exactly. You have to trust something at some point.

You have to trust something at some point.

The initial tarball of the project is fingerprinted and checked, because we trust the project itself. But subsequent downloads that the project might initiate aren't trusted. Relationship of trust isn't transitive, just like relationship of friendship isn't.

For example, in these scripts (https://github.com/QIICR/dcmqi/blob/master/CMakeExternals/DCMTK.cmake) you do request REVISION_TAG hashes, but you don't check the hash of the downloaded files. Meanwhile, nothing in the Git protocol guarantees that the downloaded files actually correspond to the revision tag. This wouldn't be true if git server is compromised, for example. So your dependency downloads aren't as secure as you might believe. -)

fedorov commented 5 years ago

So your dependency downloads aren't as secure as you might believe. -)

@yurivict this might be, but I admit I have not paid too much attention to those aspects. This has never been a priority for me. It will be great to have your input and suggestions on how we can improve things!

Having thought about this a bit more, I realized we are already building dcmqi with the pre-built dependencies on Appveyor, so I think this line is what you need: https://github.com/QIICR/dcmqi/blob/master/appveyor.yml#L58. Since you said you have all of the dependencies built, can you check if this works for you?

yurivict commented 5 years ago

I think, dcmqi isn't yet packaged by any systems. Fedora, Ubuntu, BSDs all ban downloads now for the same reasons. Only Arch doesn't, which makes Arch linux also insecure. -)

fedorov commented 5 years ago

Sorry for overlooking it earlier, but the instructions for building with the dependencies pre-built are also in the guide: https://qiicr.gitbooks.io/dcmqi-guide/user_guide/build_dcmqi.html#non-superbuild-approach. Please let us know whether this is working for you!

yurivict commented 5 years ago

The problem is with this line: -DSlicerExecutionModel_DIR:PATH=C:\SlicerExecutionModel\SlicerExecutionModel-build. It seems like SlicerExecutionModel is broken because it doesn't install anything. So I can't create a package for it.

fedorov commented 5 years ago

Ah, I see. Yes, we've been through that in https://github.com/Slicer/SlicerExecutionModel/issues/44. You should set that variable to point the build directory of SlicerExecutionModel (after you built it).

fedorov commented 5 years ago

@yurivict were you able to succeed with the build?

yurivict commented 5 years ago

@fedorov No, because SlicerExecutionModel, the required dependency, doesn't install, and I can't create a package for it.

fedorov commented 5 years ago

What is your ultimate goal? Apparently, it is not to try the software :-)

I am just curious why is it critical for you to be able to install SlicerExecutionModel. You can check out SlicerExecutionModel source code, build it, configure dcmqi to use the binaries in the build tree, and then make packages for dcmqi itself, so I don't quite understand what is the problem.

yurivict commented 5 years ago

My goal is to build SimVascular, and dcmqi is one of its dependencies.

Normally I create ports for individual software packages, so that it would be easy for others to install them later. When some project has some complications, it requires some extra steps, and makes it more difficult. I already developed the workaround for the SlicerExecutionModel problem.

The second problem was that you expect a Git repository. I disabled all Git and version related lines. It would be best if you could accept all version information through cmake variables.

Now the problem is:

In file included from /usr/local/include/ITK-4.13/itkMath.h:33:
/usr/local/include/ITK-4.13/vnl/vnl_math.h:159:5: error: 'auto' not allowed in function return type
    auto isnan(Args&&... args) -> decltype(std::isnan(std::forward<Args>(args)...)) {
    ^~~~
/usr/local/include/ITK-4.13/vnl/vnl_math.h:159:31: error: expected ';' at end of declaration
    auto isnan(Args&&... args) -> decltype(std::isnan(std::forward<Args>(args)...)) {
                              ^
/usr/local/include/ITK-4.13/vnl/vnl_math.h:159:32: error: cannot use arrow operator on a type
    auto isnan(Args&&... args) -> decltype(std::isnan(std::forward<Args>(args)...)) {
                               ^
/usr/local/include/ITK-4.13/vnl/vnl_math.h:163:5: error: 'auto' not allowed in function return type
    auto isinf(Args&&... args) -> decltype(std::isinf(std::forward<Args>(args)...)) {
    ^~~~
/usr/local/include/ITK-4.13/vnl/vnl_math.h:163:31: error: expected ';' at end of declaration
    auto isinf(Args&&... args) -> decltype(std::isinf(std::forward<Args>(args)...)) {
                              ^
/usr/local/include/ITK-4.13/vnl/vnl_math.h:163:32: error: cannot use arrow operator on a type
    auto isinf(Args&&... args) -> decltype(std::isinf(std::forward<Args>(args)...)) {
                               ^
/usr/local/include/ITK-4.13/vnl/vnl_math.h:167:5: error: 'auto' not allowed in function return type
    auto isfinite(Args&&... args) -> decltype(std::isfinite(std::forward<Args>(args)...)) {
    ^~~~
/usr/local/include/ITK-4.13/vnl/vnl_math.h:167:34: error: expected ';' at end of declaration
    auto isfinite(Args&&... args) -> decltype(std::isfinite(std::forward<Args>(args)...)) {
                                 ^
fedorov commented 5 years ago

My goal is to build SimVascular

I see. You need dcmqi because SimVascular needs MITK.

The second problem was that you expect a Git repository.

Yes, since dcmqi purpose is data conversion, it is very helpful to know what version of software was used in specific case for the purposes of debugging etc.

Now the problem is: [...]

This could be due to some compiler incompatibility. Building those packages one by one will be a lot of pain, as you've been warned in the SimVascular documentation...

Anyway, I think think the specific issue you opened is resolved. Since you have a very specific situation and are changing source code to meet your needs, I don't think it is the right place to debug the problems with the related dependencies.

If you experience problems building dcmqi following the instructions, please let us know the details and we can reopen this or create a different issue.

yurivict commented 5 years ago

The fact that I deleted .git directory and build from the tarball doesn't have anything to do with C++ errors that I've sited.

This is the error in the C++ code regardless of anything else:

error: 'auto' not allowed in function return type

fedorov commented 5 years ago

You are building a version of ITK different from the one dcmqi was checked against, and the error you mention is in ITK code, not in dcmqi, right?

fedorov commented 5 years ago

@yurivict this PR might have solved the issue you experienced: https://github.com/QIICR/dcmqi/pull/369 (partially, since now I see you also had an issue with ITK...)

I did not appreciate all the details and context earlier. If you need to build MITK, you do not need dcmqi applications (which you can disable with DCMQI_BUILD_APPS=OFF), and you indeed do not need SlicerExecutionModel. There was a bug that prevented build with apps disabled and SlicerExecutionModel not provided.

MITK was using a patched version of dcmqi, and that complicated matters a bit. Now that the bug above is resolved, with @nolden help we will hopefully switch to a dcmqi release soon, which should simplify a bit the MITK build process.

Hope this helps, and sorry I was not smart enough to link those issues earlier.