vtkSlicerVersionConfigure.h is needed for some #ifdefs that are used at compile time, so it would not be sufficient to make it available in a resource file only.
That said, a change in vtkSlicerVersionConfigure.h should not cause much rebuilding, as it is just used in a few cxx files (also in qSlicerAbstractModule.h, which could be probably removed), so it will just build and link a handful of libraries.
Could you please check if build takes a long time if you just update vtkSlicerVersionConfigure.h?
If that does not take too long then it may be something else triggers the lenghty recompilations/linkings.
Thanks for your quick answer.
Incremental build still takes the same time if I just update vtkSlicerVersionConfigure.h.
The first file recompiled is vtkMRMLVolumeSequenceStorageNode.cxx which is compiled quite quickly but then many linking must be done since it is in libMRMLCore which is used in many libs.
In the next days I will try to minimize the use of #include “vtkSlicerVersionConfigure.h” and propose a pull request.
libMRMLCore is supposed to be just an import library that loads the shared library. If the public interface of the library does not change then the import library does not change and no re-linking should occur. That’s why it is very important to avoid including vtkSlicerVersionConfigure.h in any header file. Try removing the include in qSlicerAbstractModule.h, it might solve the excessive rebuild issue.
I modify vtkMRMLVolumeSequenceStorageNode.cxx (simply add a space to a comment)
launch build in Slicer-build (make -j12)
A log screenshot is attached (I can provide the full log as a text file if you need it)
Removing vtkSlicerVersionConfigure.h in qSlicerAbstractModule.h is nice but it does not make significant change because my problem is really linking time of each library which depends on libraries modified by vtkSlicerVersionConfigure.h.
I would like to propose this improvement:
use a resource file everywhere it is possible
if information is required at build time: include vtkSlicerVersionConfigureInternal.h rather than vtkSlicerVersionConfigure.h
Removing references to vtkSlicerVersionConfigure.h in implementation (.cxx) files should not matter, as they should not trigger any relinking in other libraries. The .lib file is only modified when the public API (exported symbols in header files) is changed. When you change the content of a .cxx file then only the .so file is updated, which is not used during the linking (the content will only matter at runtime).
That said, I could reproduce the issue: touching vtkMRMLVolumeSequenceStorageNode.cxx and then running make -j20 took 52 seconds (while without this change, make took 2 seconds).
@jcfr Do you know what can cause this unnecessary re-linking?
On Windows, the build works as it is supposed to. After modifying vtkMRMLVolumeSequenceStorageNode.cxx (not just whitespace change but actual implementation change) the build time is just 3 seconds longer. If no file is modified then build takes 14 seconds (due to Python hierarchy files are updated at every build); after modifying the storage node implementation, build takes just slightly longer - 17 seconds.
@pieper could you test it on macOS? How long make -jN takes for you on a non-modified build tree and how long does it take after you make a small change in vtkMRMLVolumeSequenceStorageNode.cxx?
@aymeric.chataigner The problem can be fixed by adding set(CMAKE_LINK_DEPENDS_NO_SHARED TRUE) to the top-level CMakeLists.txt (near the top, for example right below the C++ standard section)!
If only the implementation (vtkMRMLVolumeSequenceStorageNode.cxx) is changed then rebuild takes a couple of seconds. If the header file (public interface) is changed then the rebuild takes about 30 seconds.
There is a very old discussion about enabling this flag by default. Some people brought up some contrived examples and some tests failed (maybe just on Windows?), so at that time they did not enable the CMAKE_LINK_DEPENDS_NO_SHARED flag by default, and it seems that it is still disabled by default.
@jcfr do you think we should enable CMAKE_LINK_DEPENDS_NO_SHARED or we should set target linking properties more carefully to prevent the unnecessary relinking on linux?
@aymeric.chataigner Can you confirm that building after touching vtkMRMLVolumeSequenceStorageNode.cxx just takes a couple of seconds now? That means that we solved a large part of the problem!
Cleaning up vtkSlicerVersionConfigure.h includes would be nice, it would be great if you could work on it. I think there are a couple of places where it is simply not needed.
Using a resource file will probably not be necessary, because it is simpler to get version information at runtime using qSlicerCoreApplication methods.
As its name (and comment in the file header) suggests, vtkSlicerVersionConfigureInternal.h is for internal use - only to be included in vtkSlicerVersionConfigure.h and not to be used in modules. We could rename and repurpose this .h file, but I’m not sure if it is worth it (if it can lead to significant build time reduction). What classes you would replace the include with the internal?
vtkMRMLVolumeSequenceStorageNode → The version check can be removed and we can just use the code snippet in the Slicer_VERSION_MAJOR > 4 branch.
vtkMRMLMarkupsFiducialStorageNode → it uses Slicer_VERSION, which is not available in vtkSlicerVersionConfigureInternal.h, so I think we need to keep using vtkSlicerVersionConfigure.h. It is just an implementation file in a small library, so recompiling&linking should not take more than a few seconds anyway.
qSlicerApplicationHelper → it uses Slicer_MAIN_PROJECT_VERSION_FULL, which is not available in vtkSlicerVersionConfigureInternal.h, so I think we need to keep using vtkSlicerVersionConfigure.h. If you find the recompilation of this file makes a huge difference then we can have a look how to avoid it.
I proposed these PR to continue to save more time during incremetal build:
On my PC with ubuntu 20.04 running an intel i7 6 hyperthreaded cores:
If I do make -j12:
Build slicer incremental: 35 seconds rather than 40 seconds
Build slicerRT incremental: 30 seconds rather than 1 minute