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.

Sign up for GitHub

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

Jump to bottom

Update minimum required cmake #765

Closed
wants to merge 2 commits into from
Closed

Update minimum required cmake #765

wants to merge 2 commits into from

Conversation

jcfr
Copy link
Member

@jcfr jcfr commented Aug 3, 2017 •
edited
Loading

NOTE: The topic include two commits to facilitate review. I will squash the two commits before integration.

@jcfr
COMP: Update CMake minimum required version from 2.8.9 to 3.5
2efbc9c 
This commit removes few workarounds and explicit setting of CMake policies.

It also ensures the required CMake version support VS 2015 (introduced in CMake 3.1)

CMake 3.5 is also old enough (release in April 2016) and available (same version
or above) in distributions like these ones:

* Arch Linux
* Ubuntu LTS 14.04, 16.04
* OpenSUSE Leap 42.2, 42.3
* Debian 9, Sid
* Fedora 24/25/26
* Slackware 14.2

See https://pkgs.org/download/cmake
@jcfr
COMP: cmake_minimum_required -> 3.5
94281a8
lassoan
lassoan reviewed Aug 4, 2017
View reviewed changes
Copy link
Contributor

@lassoan lassoan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Do we need to update required CMake version in all extensions?

CMake/SlicerExtensionCPack.cmake
@@ -187,7 +187,7 @@ if(APPLE)
# for SuperBuild extensions.

file(WRITE ${slicer_extension_cpack_bundle_fixup_directory}/CMakeLists.txt
"cmake_minimum_required(VERSION 3.0)
"cmake_minimum_required(VERSION 3.3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentionally just 3.3 and not 3.5?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. This one should be updated.

@jcfr
Copy link
Member Author

jcfr commented Aug 4, 2017

Looks good. Do we need to update required CMake version in all extensions?

The statement cmake_minimum_required(VERSION X.Y) does NOT need to be updated in the extension source tree.

That said:

  • whenever possible, extension developer are encouraged to update it in the top level CMakeLists.txt. This will most likely avoid confusion ...
  • the version of CMake used to build the extension would need to be 3.5. To facilitate the overall maintenance, I made this is a hard requirement by adding explicit statement cmake_minimum_required(VERSION 3.5) to SlicerConfig.cmake and UseSlicer.cmake

dzenanz
dzenanz reviewed Aug 4, 2017
View reviewed changes
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides confusion over whether it should be CMake 3.3 or 3.5, it looks good.

@dzenanz
Copy link
Member

dzenanz commented Aug 4, 2017

Oops, I forgot there is the second commit which changes 3.3 -> 3.5.

jcfr referenced this pull request in jcfr/MultiVolumeExplorer Aug 4, 2017
@jcfr
COMP: Update CMake version to 3.5.0
cd244f4 
This commit removes explicit setting of CMake policies.

Note that the setting of CMP0017 to OLD is not needed.

See Slicer/Slicer#765
@jcfr jcfr mentioned this pull request Aug 4, 2017
Merged
jcfr referenced this pull request in fedorov/MultiVolumeImporter Aug 4, 2017
@jcfr
COMP: Update CMake version to 3.5.0
b18153e 
This commit removes explicit setting of CMake policies.

Note that the setting of CMP0017 to OLD is not needed.

See Slicer/Slicer#765
@jcfr jcfr mentioned this pull request Aug 4, 2017
Merged
jcfr referenced this pull request in Slicer/BRAINSTools Aug 4, 2017
@jcfr
COMP: Removes explicit setting of CMake policy CMP0017.
cc09cbe 
Setting of CMP0017 to OLD is not needed.

See Slicer/Slicer#765
jcfr referenced this pull request in Slicer/BRAINSTools Aug 4, 2017
@jcfr
COMP: Fix warning removing explicit setting of CMake policy CMP0017.
55f7e32 
Setting of CMP0017 to OLD is not needed.

This commit addresses the following warning reported when using CMake 3.9.0:

```
CMake Deprecation Warning at /path/to/Slicer-build/BRAINSTools/CMakeLists.txt:11 (cmake_policy):
  The OLD behavior for policy CMP0017 will be removed from a future version
  of CMake.

  The cmake-policies(7) manual explains that the OLD behaviors of all
  policies are deprecated and that a policy should be set to OLD only under
  specific short-term circumstances.  Projects should be ported to the NEW
  behavior and not rely on setting a policy to OLD.
```

See Slicer/Slicer#765
slicerbot referenced this pull request Aug 4, 2017
@jcfr
COMP: Update EMSegment: add VTKv8 support
39a8540 
Also remove CMake warnings and fix module category


More details below:

------------------------------------------------------------------------
r17134 | COMP: Fix warning removing explicit setting of CMake policy CMP0017.

Setting of CMP0017 to OLD is not needed.

This commit addresses the following warning reported when using CMake 3.9.0:

```
CMake Deprecation Warning at /home/jcfr/Projects/Slicer-2-build/EMSegment/CMakeLists.txt:12 (CMAKE_POLICY):
  The OLD behavior for policy CMP0017 will be removed from a future version
  of CMake.

  The cmake-policies(7) manual explains that the OLD behaviors of all
  policies are deprecated and that a policy should be set to OLD only under
  specific short-term circumstances.  Projects should be ported to the NEW
  behavior and not rely on setting a policy to OLD.

```

See Slicer/Slicer#765

------------------------------------------------------------------------
r17133 | lassoan | COMP: Setting CMP0020 to NEW to remove build warnings

------------------------------------------------------------------------
r17132 | lassoan | ENH: Fixed IslandRemoval module category

It was "Filter", while the correct category name is "Filtering"

------------------------------------------------------------------------
r17131 | msmolens | ENH: Call InitializeObjectBase() in New() methods

In VTK8 it's necessary for New() methods to call InitializeObjectBase() on the
new object for proper tracking with vtkDebugLeaks. The standard macros
(vtkStandardNewMacro, vtkObjectFactoryNewMacro) handle this. For those classes
that don't use the macros, add calls to InitializeObjectBase().

Continue to support earlier versions of VTK by wrapping calls to
InitializeObjectBase() in checks for the preprocessor define
VTK_HAS_INITIALIZE_OBJECT_BASE.

It's possible that many of these implementations could be simplified by using
the standard macros.

See also:
- Kitware/VTK@e5c793d
- http:https://public.kitware.com/pipermail/vtk-developers/2016-September/034332.html

git-svn-id: http:https://svn.slicer.org/Slicer4/trunk@26203 3bd1e089-480b-0410-8dfb-8563597acbee
@jcfr
Copy link
Member Author

jcfr commented Aug 4, 2017

Thanks everyone for the review.

Integrated in r26201, r26202 and r26203

Associated forum topic: https://discourse.slicer.org/t/minimum-required-version-of-cmake-updated-to-3-5-0/828

@jcfr jcfr closed this Aug 4, 2017
@jcfr jcfr deleted the update-minimum-required-cmake branch August 4, 2017 19:18
jcfr referenced this pull request in jcfr/SlicerGitSVNArchive Aug 10, 2017
@jcfr
COMP: Update EMSegment: add VTKv8 support
0f800fe 
Also remove CMake warnings and fix module category


More details below:

------------------------------------------------------------------------
r17134 | COMP: Fix warning removing explicit setting of CMake policy CMP0017.

Setting of CMP0017 to OLD is not needed.

This commit addresses the following warning reported when using CMake 3.9.0:

```
CMake Deprecation Warning at /home/jcfr/Projects/Slicer-2-build/EMSegment/CMakeLists.txt:12 (CMAKE_POLICY):
  The OLD behavior for policy CMP0017 will be removed from a future version
  of CMake.

  The cmake-policies(7) manual explains that the OLD behaviors of all
  policies are deprecated and that a policy should be set to OLD only under
  specific short-term circumstances.  Projects should be ported to the NEW
  behavior and not rely on setting a policy to OLD.

```

See Slicer/Slicer#765

------------------------------------------------------------------------
r17133 | lassoan | COMP: Setting CMP0020 to NEW to remove build warnings

------------------------------------------------------------------------
r17132 | lassoan | ENH: Fixed IslandRemoval module category

It was "Filter", while the correct category name is "Filtering"

------------------------------------------------------------------------
r17131 | msmolens | ENH: Call InitializeObjectBase() in New() methods

In VTK8 it's necessary for New() methods to call InitializeObjectBase() on the
new object for proper tracking with vtkDebugLeaks. The standard macros
(vtkStandardNewMacro, vtkObjectFactoryNewMacro) handle this. For those classes
that don't use the macros, add calls to InitializeObjectBase().

Continue to support earlier versions of VTK by wrapping calls to
InitializeObjectBase() in checks for the preprocessor define
VTK_HAS_INITIALIZE_OBJECT_BASE.

It's possible that many of these implementations could be simplified by using
the standard macros.

See also:
- Kitware/VTK@e5c793d
- http:https://public.kitware.com/pipermail/vtk-developers/2016-September/034332.html

git-svn-id: http:https://svn.slicer.org/Slicer4/trunk@26203 3bd1e089-480b-0410-8dfb-8563597acbee
LunarMedic referenced this pull request in LunarMedic/EMSegment Nov 27, 2017
@jcfr
COMP: Fix warning removing explicit setting of CMake policy CMP0017.
08758d7 
Setting of CMP0017 to OLD is not needed.

This commit addresses the following warning reported when using CMake 3.9.0:



See Slicer/Slicer#765
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lassoan lassoan lassoan left review comments

@dzenanz dzenanz dzenanz left review comments

@pieper pieper pieper approved these changes

@msmolens msmolens msmolens approved these changes

@ihnorton ihnorton Awaiting requested review from ihnorton

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
5 participants
@jcfr @dzenanz @pieper @lassoan @msmolens