CMake update

Engineering
4

Thanks for working on it Jon!

1 Like
5

The dashboard build machines need to have their CMake version updated to at least 3.10.2 before that patch is merged. @seanm @blowekamp @fbudin @hjmjohnson can you check you build machines and give a green light when ready?

6

Unless this addresses a bug, this will be a low priority for me to update all our machines.

7

I’m ready!

Hans

1 Like
8

PRs to set the cmake_minimum_required to 3.10.2 for the remote modules:

Additionally, the following have a minimum ITK VERSION requirement as well, e.g.

  find_package(ITK 4.9 REQUIRED)

Do these need to be update to ITK 5.0?

Also, do the CMake policies present across the CMakeLists.txt files need to be updated? If, yes, which is the policy to be enfored?

Once the PRs accepted, I’ll submit a topic to gerrit to update the commit hashes.

Thanks.

1 Like
9

WOW! Great work.

10

I don’t remember why we added minimum version to RLEImage, but I don’t think it needs to be updated.

11

Awesome, Jon!

I do not think we should require ITK 5.0 unless it is really required, e.g. the ITK 5.0 C++11 scripts have been executed. This allows folks to still use the modules with older versions of ITK.

Many of the CMake policies can be removed because they default to NEW when a newer version of CMake is required.

12

The GitHub external module CI image, insighttoolkit/module-ci:latest has been updated to CMake 3.11.1.

13

Great! @hjmjohnson @matt.mccormick you can update ISC ITKIsotropicWavelets from upstream. It includes cmake update, and the recently merged itkViewImage is removed.

The ITKv4 compatible is the branch: https://github.com/phcerdan/ITKIsotropicWavelets/tree/ITK4.13-Compatible

14

@phcerdan Should we just remove the fork?

15

That is a good discussion to have for remote modules in general (pinging @jhlegarreta). My personal opinion is that if the original author is active the InsightSoftwareConsortium fork just creates extra maintenance burden. But maybe don’t remove it from ISC, and keep it alive to open Pull Requests from there to the original repo, in case a general update for all modules is required (as @hjmjohnson did with the c++11 update).

16

All remotes have gone through the C++11 update process :-S

Awesome. Thanks !

Yes, this is something that should be agreed upon to set a policy. @phcerdan is right in that it creates extra maintenance if the author is active. I tend to think that forks outside ISC and which are not active do also create extra maintenance and delays :slight_smile:

Then we should agree what being active means in this respect: 6-months, or 1-year without commits?

Not skilled at this, but is it feasible to think about some script for these maintenance/sync/upstream pull issues?

OTOH, is it advised to have the cmake_minimum_required updated for these ITK(Kitware)-related projects as well?

1 Like
17

This is a good idea. How about 1 month?

We could bump KWStyle.

Another option for ITKIsotropicWavelets is to transfer the repository to the InsightSoftwareConsortium.

18

I will be happy about this if I keep admin privileges on this one!

1 month without commits seems too short to me if the repo is stable. I mean, the maintainer can be active, but not actively working on the external module because there are no open issues, etc.

1 Like
19

Great! Certainly, you will keep admin privileges. Since you are member of the InsightSoftwareConsortium GitHub Org, I think you should be able to transfer the repository, according to this documentation. Please let us know if you have any issues.

Agreed – fork if no activity in over 1 year?

1 Like
20

Sounds good, and we can open an issue and ask directly to confirm.

Cool, I will do it next week.

1 Like
21

Seems reasonable to me. Just to avoid confusion: the activity is over the repo and no the user themselves?

Once this clarified, I’ll make a note in the documentation and post here.

Will do.

BTW:

I put a note in the ITK issue I opened.

For the sake of completeness, and to honor the subject, the gerrit topic to update the remote hashes on the CMake update:
http://review.source.kitware.com/#/c/23407/

1 Like
22

A PR was submitted to the KWStyle repository to update its cmake_minimum_required version to CMake 3.10.2.

All remote modules’ cmake_minimum_required upgrade PRs have been merged, and their new commit hash update gerrit topic has been merged into ITK master.

2 Likes
23

We are almost done updating all out machines to CMake 3.11.1 from 3.10.1, just mini 7 left which should be addressed today.

2 Likes