BUG: Fix module ITK discovery as a remote built with Examples.

[jhlegarreta]

Fix the Examples/CMakeLists.txt to avoid the module asking for ITK_DIR
because it couldn't fint ITKConfig.cmake if built as a remote module and
if the BUILD_EXAMPLES flag is set to ON.

[fbudin69500]

I haven't tested yet the fix, but other extensions may have to be updated if this fix works:

• ITKFixedPointInverseDisplacementField
• ITKMinimalPathExtraction

[thewtex]

Thanks for the updates, Jon!

I added more details inline.

[thewtex]

I think we only want (untested):

if(NOT ITK_SOURCE_DIR)
  find_package(ITK REQUIRED COMPONENTS PrincipalComponentsAnalysis)
  include(${ITK_USE_FILE}
else()
  include_directories( ${PrincipalComponentsAnalysis_SOURCE_DIR}/Source )
endif()

This is an example -- in should not call itk_module_impl()

[fbudin69500]

Just as a comment: I think, at least for some extensions, the 'example' directory was made to be built as a different project. Is is still the case? Or are the examples now build from the main directory?

[thewtex]

I think, at least for some extensions, the 'example' directory was made to be built as a different project. Is is still the case?

Yes, I think so.

[jhlegarreta]

Not sure what to include in the else statement:
include_directories(${PCA}/include)
or
include_directories(${PROJECT_SOURCE_DIR}/include)
results in linking errors since the ITK's additional dependencies do not get set.

[fbudin69500]

@jhlegarreta : What is below is what I wanted to post yesterday but apparently I forgot to press a button... I have tested the branch that share the link with outside of ITK and it works, but inside ITK there are linking errors. It might be the problem you have also encountered.

I have create a branch in my repo that modifies slightly your implementation to use @thewtex 's idea and corrects some problems with missing 'typename':
https://github.com/fbudin69500/ITKPrincipalComponentsAnalysis/tree/jhlegarreta-FixFindITKWhenUsingAsITKRemoteModule-for-Release
I tested it outside of ITK and it works (your branch was throwing some error)

[fbudin69500]

@jhlegarreta @thewtex : I have updated Jon's branch in my repo and I think it works both inside and outside of ITK now [1].

[1] https://github.com/fbudin69500/ITKPrincipalComponentsAnalysis/commits/jhlegarreta-FixFindITKWhenUsingAsITKRemoteModule-for-Release

[jhlegarreta]

Then I'm puzzled.

Mine also worked outside, but not inside, as in your previous comment. Well, indeed, leaving
itk_module_impl()
in the else clause works outside as well; but as said, if built as a remote, ITK libraries are not listed for the linker.

But note that in my first commit within this topic I removed any reference to /Sources, since it does not exist as a folder within the tree. Thus, I do not see what that lists or includes. So how is that working?

[jhlegarreta]

@fbudin69500 @thewtex I've push forced a new commit.

@fbudin69500 as for the other remotes, thanks for bringing those issues to surface ! I'd propose to first fix this, and once we see that the behavior is as expected, we could tackle those two remote modules. I think the Cuberille module has also the same issue.

@thewetex thanks for the suggestion. This last commit does not yet solve the linker library issues commented by François and me.

However, I brought some fixes given that you would sooner or later find them when checking out/forking the branch.

So the summary of this commit is this: Matt's comments, along with testing on another machine, uncovered a couple of other issues :-S

I've realized that when building outside the ITK tree (i.e. as an independent project) the BUILD_EXAMPLES and BUILD_DOCUMENTATION flags only appear if their ITK counterparts are set ON in the ITK source tree. This is clear from the root CMakeLists.txt and the use of CMAKE_DEPENDENT_OPTION. I guess this is not what we want if building outside ITK.

Thus, I've changed the root CMakeLists.txt as well.

Also, and although this is may be a less important question, I guess we do not want the Module_{ModuleName}_ prefix to be added to the flags when building outside ITK. I did the changes taking this into account.

The code is may be redundant due to this. Suggestions are welcome.

BTW, I think the cache issue mentioned yesterday is not/no longer an issue: unchecked ITK's BUILD_EXAMPLES and when building as a remote module, the module's flag is no longer present !

I updated the commit message.

But as discussed earlier, the linking problem when built as a remote is yet unsolved.

[thewtex]

Moving forward :-)

Should we merge this branch, then integrate @fbudin69500 's patch?

[jhlegarreta]

I wouldn't merge it if we know that it indeed causes issues when built as a remote. If @fbudin69500 's patch works, I'd rather rebase his on top of this, and eventually merge his and close this.
But as said, I'm curious about the use of the /Source folder as a fix if the tree does not have any such folder.

[fbudin69500]

@jhlegarreta I just tested your last patch and it doesn't build outside of the source tree (building the test fails). Do you mind testing the branch I sent?

[jhlegarreta]

@fbudin69500 You're right, my last patch set fails to compile outside, at least in the first build; but in a second run, voilà it compiles. Weird. I'd need to investigate why :-S Sorry.

I've tested yours and it seems to have the same behavior; the first one fails as mine, due to a mistery PrincipalComponentsAnalysisHeaderTestClean project, which shows up in the solution. I haven't found any reference to it in the CMakeLists. And I don't recall having seen it until today. Weird.

Well, kind of late here. I'll have a look at it tomorrow.

[jhlegarreta]

The weird PrincipalComponentsAnalysisHeaderTestClean project is not listed in MSVC 2010. Guess that is related to the IDE, so may be not at the core of the problem.

I've been struggling to get the list of linked ITK libraries automatically, and have re-read
https://github.com/InsightSoftwareConsortium/ITK/blob/master/CMake/ITKModuleAPI.cmake
https://github.com/InsightSoftwareConsortium/ITK/blob/master/CMake/ITKModuleMacros.cmake

and have made several unsuccessful attempts.

May be we should list the link dependencies for the example in the module's itk-module.cmake ?

Now, I don't know which is the variable where the dependencies should be added, and whether that will effectively work.

The compiler complains about vnl-related libraries, so may be adding them to the itk-module.cmake DEPENDS list would do the trick?

I tried to add them without success. Surely enough I did not do it the right way.

When building externally, the following are the libs listed in ITK_LIBRARIES:
ITKCommon ITKIOImageBase ITKIOMesh ITKMesh itksys itkvnl_algo itkvnl itkv3p_netlib itknetlib itkvcl
And my linker shows additionally:
ITKgiftiio ITKEXPAT ITKniftii ITKznz itkzlib ITKTransform itkdouble-conversion ITKVNLInstantiation

And that said, the module's test project has way more than those, all those in ITK_LIBRARIES (?).

The Cuberille remote module's example CMakeLists, although I fear is not completely well at this level, lists its link libraries manually (ITKQuadEdgeMesh). Now, the truth is that the project does not later show up in my IDE, so that is another issue to be solved when tackling that module.

[thewtex]

Another idea:

Instead of

itk_module_impl()

itk_module_test()
set(ITK_LIBRARIES ${${PCA}-Test_LIBRARIES})

(untested)

[jhlegarreta]

That was nice !

Although it works, I've got a couple of concerns:

• Assimilating CMake macros and variables that were intended for tests in examples seems counter-intuitive to me.
• If ITK's BUILD_TESTING is set to OFF, the approach no longer works.

I'd propose adding an itk_module_example() macro to ITK/CMake/ITKModuleMacros.cmake copying what is done for itk_module_test() and adapting the variable names. I ignore whether this can have other undesired side-effects.

[thewtex]

+1 for itk_module_example()

[fbudin69500]

Should we worry about an itk_module_example() macro if examples are expected to be stand-alones?
I think it would make sense to make a decision whether or not examples are suppose to be build as part of the project or build independently such as what is in the examples.

[thewtex]

I do think it is important to have the modules standalone.

For testing the examples, it much easier to use the same project build then to need to set up a second example build. Like the Sphinx examples, maybe we should have itk_module_example() or its equivalent content in ITKPrincipalComponentsAnalysis/CMakeLists.txt instead of ITKPrinicipalComponentsAnalysis/examples/CMakeLists.txt (if that can be configured).

[jhlegarreta]

I may have misunderstood something, so correct me if I'm wrong.

Are we talking about integrating a remote module's example into the ITKExamples system?

I'd be for having a separate project for a remote module example, as done right now. I guess that remotes are intended tot become part of the core at some point in time, as with the previous review system. Hence, the moment it is decided to integrate a remote into ITK would be the time to add the example to the Sphinx/ITKExamples system.

So I'd vote for adding an itk_module_example() macro to solve the current situation. I wouldn't have the example file be listed in the same project as the sources/includes either (if that would be achieved by setting the itk_module_example() in the ITKPrincipalComponentsAnalysis/CMakeLists.txt file).

But as said, I may have misunderstood something. It may be worthwhile discussing this with other developers at Kitware, and whenever you reach a consensus, we can go for the implementation.

[fbudin69500]

Jon, What we are envisioning is to have a clean and simple example directory that could be copied outside of the remote module folder and built independently. This would allow users to start a new project based on the example. The example folder could still be build inside the remote module, both from within ITK and outside of ITK. This will require a few updates in ITKConfig.cmake for which I am going to create a patch. Francois

…

On Thu, May 11, 2017 at 5:58 PM, Jon Haitz Legarreta Gorroño < ***@***.***> wrote: I may have misunderstood something, so correct me if I'm wrong. Are we talking about integrating a remote module's example into the ITKExamples system? I'd be for having a separate project for a remote module example, as done right now. I guess that remotes are intended tot become part of the core at some point in time, as with the previous review system. Hence, the moment it is decided to integrate a remote into ITK would be the time to add the example to the Sphinx/ITKExamples system. So I'd vote for adding an itk_module_example() macro to solve the current situation. I wouldn't have the example file be listed in the same project as the sources/includes either (if that would be achieved by setting the itk_module_example() in the ITKPrincipalComponentsAnalysis/CMakeLists.txt file). But as said, I may have misunderstood something. It may be worthwhile discussing this with other developers at Kitware, and whenever you reach a consensus, we can go for the implementation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYZGNz6EV4ivbaZMwgWpf8qw9QsNe1Qks5r44RrgaJpZM4NV7IK> .

[fbudin69500]

Here is a branch to be tested for ITK:
http://review.source.kitware.com/#/c/22358/
and the corresponding PCA branch is available here:
https://github.com/fbudin69500/ITKPrincipalComponentsAnalysis/tree/SimplifyExampleDirectoryConfiguration

ITKPrincipalComponentsAnalysis.cmake.remote needs to be hacked to point to the correct WIP branch of ITKPrincipalComponentsAnalysis

[fbudin69500]

@jhlegarreta I finally merged the patch to easily add examples in remote modules: InsightSoftwareConsortium/ITK@3409d08

[hjmjohnson]

@jhlegarreta Is this PR still relevant?

[jhlegarreta]

Sorry for lagging behind this.

@hjmjohnson This stems from the fact that when requesting to build a remote module's documentation and/or examples, the were not being nested under the ModuleName, which I found weird. Furthermore, the documentation flag corresponded, in reality, to the IJ article, which I found misleading if compared to the BUILD_DOCUMENTATION flag.

Now, we attempted at several times to get a solution working, but turned out not to be so easy. It was still in my ToDo list to come back to this when I found the necessary energy.

[hjmjohnson]

@jhlegarreta Reminder. Perhaps we should close this pull request and make an issue to be addressed later.

[jhlegarreta]

Closing the PR due to lack of time to work on na solution and opening #19 to keep track of this.