New Build Errors on CDash 1/24/18 - drop support for VS2013 in ITKv5?

Engineering
21

Aspiring to Qt’s level of support is a worthy goal, but quite unrealistic considering that only a few people are working part time on maintenance of ITK. Therefore, we need to prioritize.

22

Our continued support is not necessary for ITK 4.13 on VS2013. It works now, and will continue to work as it does today. We SHOULD NOT change ITK 4.13.

23

Thanks for the reminder. I have updated the title of this thread to be more specific. This thread is only talking about ITKv5 and ITKv4.13 still supports the same list of compilers than it was originally designed to support, including Visual Studio 2013.

24

@Niels_Dekker: Since you are the person (in this thread) who is going to have the most trouble with this change, could you elaborate on the the reason that force you to use Visual Studio 2013? It would be good to have more details before we make a final decision. Are you in control of the infrastructure that you are using? Are you required to use an old version of MeVisLab? Are these requirements likely to change in the coming months? Would it be a complete blocker for you to switch to ITKv5 if Visual Studio >= 2015 is required? We do not want to end up having a split in the ITK community with half of the people stuck on ITKv4.13 and the other half using ITKv5. That would be a bad scenario.

25

@fbudin Surely I hope to upgrade MeVisLab very soon. But it’s not that easy. We have a lot of MeVisLab specific modules and networks here at LKEB (www.lkeb.nl) that may not work as expected after a major MeVisLab upgrade.

1 Like
26

Some considerations to support for legacy ITKv4:

  • Debian was packaging ITKv3 libinsightoolkit3 up to Jessie released April 2015 (LTS from June 2018 to end of April 2020). ITKv4 will likely have a long tail of usage on the LTS linux distributions
  • Additionally, we now have remote modules, while ITKv4 may not get feature updates, we need to consider that people may want or need to update their remote modules for ITKv4. Maybe like a Slicer module can be updated for a release?
  • We also need to consider ITKv4 support with the new GitHub workflows. I presume we will still need to be able to support testing critical patches for ITKv4 release in the new GitHub paradigm.

Looking over other C++11 features that VS12 2013 is missing that may be important is the noexcept keyword.

1 Like
27

Those are some good arguments for providing bug fix patches for ITKv4.

@Niels_Dekker has made valuable contributions to ITKv5 / master. We want him, and others like him, to continue this excellent practice. For folks to scratch their own itches, they will need to be able to use ITK master in their development environment. It sounds like this includes VS 2013 for the time being.

If all that is required to re-enable basic VS 2013 support is a revert of this patch, is seems worth it.

28

@matt.mccormick That patch by itself does not seem especially important, so we could revert it for now. But we could have a list of patches which require dropping VS2013 support, and merge them later, closer to 5.0 feature freeze. This will give time to @Niels_Dekker and potentially others to update/upgrade the rest of their software.

1 Like
29

The developer guidelines will also need to specify that constexpr cannot
be used for now, and that the equivalent ITK macros need to be used instead.

1 Like
30

I believe we will also need to continue the use of ITK_NOEXCEPT_OR_THROW, or whatever it is, was, should be. I believe it will need to be defined as NOEXCEPT for C++11 compatible compiler and throw for legacy.

31

FYI With the help of a few of my colleagues at LKEB, I made considerable progress, preparing to upgrade my MeVisLab project to VS2015 (+ MeVisLab 3.0), in the last few days. So I’m still hopeful to be able to drop VS2013 soon.

But I also posted this message to check that https://discourse.itk.org is back online again! It seemed to be down in the last few hours :open_mouth:

1 Like
32

Thanks @Niels_Dekker ! This is great news! And indeed, discourse was down from yesterday evening to this morning. Thanks to our sys-admins, a tentative alarm system has been created so that we are made aware of that problem automatically.

1 Like
33

The patch that I just submitted, ENH: Added HoughTransform2D Circle class… http://review.source.kitware.com/#/c/23139 was originally rejected by AppleClang 6.0.0.6000056 at Dash5.kitware.com, while it was accepted by AppleClang 8.0.0.8000042 at misty.kitware.com, and also by the other Kitware Build Robot compilers. The code rejected by the older version of Clang was as follows:

template< typename TInputPixelType, typename TOutputPixelType, typename TRadiusPixelType >
HoughTransform2DCirclesImageFilter< TInputPixelType, TOutputPixelType, TRadiusPixelType >
::Circle::Circle(const CenterType& center, const RadiusType radius)
  :
  m_Center{ center }, //  <===  error: no viable conversion...
  m_Radius{ radius }
{
}

AppleClang 6.0.0.6000056 error message: “error: no viable conversion from ‘const CenterType’ (aka ‘const Index<2>’) to ‘IndexValueType’ (aka ‘long’)”

Should ITKv5 still support this old version of Clang?

BTW, I worked around this compiler issue by using parentheses instead of curly braces to initialize m_Center: Patch Set 7, http://review.source.kitware.com/#/c/23139/7

34

According to:
https://trac.macports.org/wiki/XcodeVersionInfo

that AppleClang is from Xcode 6.1.1.

According to:
https://en.wikipedia.org/wiki/Xcode#6.x_series

6.1.1 was released 2014-12-02, which is not all that old.

The next Xcode (6.2, 2015-03-09) was the last to support running on OS X 10.9. I would be against dropping support for 10.9 (and thus Xcode 6.2).

That bot (Dash5) could probably be updated to Xcode 6.2, but I doubt it would fix the error, as the compiler version is only slightly newer.

Cheers,

35

Thanks for your reply, @seanm. It appears that the issue was fixed between clang 3.6.0 and clang 3.7.0. Here is a minimal example:

struct Foo { int data[2]; };

class Bar
{
    Foo m_foo;
public:
    Bar(const Foo& arg)
    :
    m_foo { arg } // clang 3.6 error: no viable conversion from 'const Foo' to 'int'
    {
    }
};

The example compiles well at http://rextester.com/IYUC73647 (clang 3.8.0), and also at https://godbolt.org when you select clang 3.7.0 or higher, and add -std=c++11 as compiler option. However, it fails at https://godbolt.org when you select clang 3.6.0 or an earlier version, saying error: no viable conversion from ‘const Foo’ to ‘int’.

36

Alas godbolt doesn’t have AppleClang as an option, but my 10.9 bot (RogueResearch12) uses Xcode 6.2 (newest that runs on 10.9, like I said) and it gives the same error:

builder12:~ builder$ cc --version
Apple LLVM version 6.0 (clang-600.0.57) (based on LLVM 3.5svn)
Target: x86_64-apple-darwin13.4.0
Thread model: posix

builder12:~ builder$ cc -fsyntax-only -std=c++11 ~/Desktop/test.cxx
/Users/builder/Desktop/test.cxx:9:17: error: no viable conversion from ‘const Foo’ to 'int’
m_foo { arg } // clang 3.6 error: no viable conversion from ‘const Foo’ to ‘int’
^~~

37

FYI, When I submitted http://review.source.kitware.com/#/c/23228/1 Dash5.kitware.com (Kitware Build Robot) produced an error from MacOSX-Clang/AppleClang 6.0.0.6000056 at https://open.cdash.org/viewBuildError.php?buildid=5282174 saying:

error: default initialization of an object of const type ‘const NeighborhoodInnerProduct<InputImageType, double>’ requires a user-provided default constructor

This appears to be an old Clang compiler issue, which appears to occur with clang 3.8.1 and older, and is fixed with clang 3.9.0. Here is an simple example where the old clang produces the error:

struct Foo {};

constexpr Foo constexprFoo; // error: default initialization of an object of const type...

See also http://rextester.com/OHZOJQ69129

A workaround would be to just add = {}:

constexpr Foo constexprFoo = {};  // Accepted by any Clang 3.X (I guess).