Skip to content

ENH: Update compiler versions for C++14#156

Merged
thewtex merged 2 commits intoInsightSoftwareConsortium:masterfrom
N-Dekker:Upgrade-compiler-versions-for-C++14
Jun 10, 2021
Merged

ENH: Update compiler versions for C++14#156
thewtex merged 2 commits intoInsightSoftwareConsortium:masterfrom
N-Dekker:Upgrade-compiler-versions-for-C++14

Conversation

@N-Dekker
Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker commented Jun 1, 2021

Following pull request InsightSoftwareConsortium/ITK#2563
"ENH: Upgrade ITK from C++11 to C++14"

Suggested by Jon Haitz Legarreta Gorroño (@jhlegarreta) at InsightSoftwareConsortium/ITK#2563

Copy link
Copy Markdown
Member

@jhlegarreta jhlegarreta 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 to me. I'd say that wisest would be to wait until the changes are agreed upon and merged in ITK before merging.

Comment thread SoftwareGuide/Latex/Introduction/Installation.tex Outdated
@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Jun 1, 2021

Note to maintainers: this pull request should only be merged once ITK has indeed moved from C++11 to C++14 (possibly by pull request InsightSoftwareConsortium/ITK#2563 ), as commented by @jhlegarreta

@thewtex
Copy link
Copy Markdown
Member

thewtex commented Jun 3, 2021

Thanks for adding this @N-Dekker !

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Jun 4, 2021

@thewtex You're welcome, but I'm not entirely sure if my pull request is OK. Looking at:

%% The following compilers should have nightly dashboard in the "Expected builds"
%% to ensure that support is maintained.
%%
\begin{itemize}
\item GCC 4.8 and newer
\item Visual Studio 2015, 2017, 2019, toolsets 14.x, 15.0, 16.0
\item Apple Clang 7.0 and newer
\item Clang 6 and newer
\end{itemize}

Versus:

https://github.com/InsightSoftwareConsortium/ITK/blob/master/Documentation/SupportedCompilers.md

Should they be the same? Because now they are different! Even after my pull request 🤔

Apparently ITK already required AppleClang 7.0.2 and later (from Xcode 7.2.1) before the upgrade to C++14! Should that requirement still be adjusted for C++14?

Following pull request InsightSoftwareConsortium/ITK#2563
"ENH: Upgrade ITK from C++11 to C++14"

Suggested by Jon Haitz Legarreta Gorroño
@N-Dekker N-Dekker force-pushed the Upgrade-compiler-versions-for-C++14 branch from fc64bda to 80b8f89 Compare June 8, 2021 09:41
Comment on lines 38 to 39
\item Apple Clang 7.0 and newer
\item Clang 6 and newer
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this PR ("ENH: Update compiler versions for C++14") does not need to adjust the Clang versions, as Clang 6 and Apple Clang 7.0 should already support C++14. Right, @seanm ? Note that these are the compilers that "should have nightly dashboard". ITK might still compile fine on other compiler versions 😃

I'm not against upgrading Clang, it's just that it seems beyond the scope of this pull request.

@thewtex
Copy link
Copy Markdown
Member

thewtex commented Jun 8, 2021

Should they be the same? Because now they are different!

Yes, could they please be synchronized?

@N-Dekker N-Dekker marked this pull request as ready for review June 8, 2021 19:12
@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Jun 9, 2021

@thewtex

Yes, could they please be synchronized?

They may not mean exactly the same thing: The compilers that "should have nightly dashboard" and the "supported" compilers. Or should they be exactly the same compilers?

Anyway, this pull request does already synchronize MSVC and GCC. For (Apple) Clang I just do not know the preferred minimum. We AppleClang 12.0.0.12000032 Xcode 12.4 (on GitHub Actions) and Apple clang version 12.0.5 (clang-1205.0.22.9) (as @ViktorvdValk told me). Sean (@seanm) wrote at https://discourse.itk.org/t/may-we-start-using-c-14-at-the-master-branch/4172/23?u=niels_dekker that he is ok with Xcode 7.2.1 being the minimum.

@seanm
Copy link
Copy Markdown

seanm commented Jun 9, 2021

You may as well put Xcode 7.2.1 as the minimum.

It's the newest possible Xcode on macOS 10.10.x, which is the oldest version of macOS that I have a bot for. I don't think anyone else runs any Mac bot older than that.

Suggested by Sean McBride, as he wrote about Xcode 7.2.1:
> It's the newest possible Xcode on macOS 10.10.x, which is the oldest
> version of macOS that I have a bot for.

Details taken from
https://trac.macports.org/wiki/XcodeVersionInfo#Xcode7.2.11
@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Jun 9, 2021

You may as well put Xcode 7.2.1 as the minimum.

Thanks @seanm Please check f65f786 !

@thewtex
Copy link
Copy Markdown
Member

thewtex commented Jun 9, 2021

They may not mean exactly the same thing: The compilers that "should have nightly dashboard" and the "supported" compilers. Or should they be exactly the same compilers?

They should be the same compilers. We likely could use more dashboard builds, but that is a separate issue.

@thewtex thewtex merged commit cafb782 into InsightSoftwareConsortium:master Jun 10, 2021
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Jun 10, 2021
Following commit InsightSoftwareConsortium/ITKSoftwareGuide@f65f786
"ENH: Update Xcode Apple Clang compiler for nightly dashboard"

The new version numbers are based on feedback from Sean McBride, at:

Pull request "ENH: Update compiler versions for C++14"
InsightSoftwareConsortium/ITKSoftwareGuide#156

Discussion topic "May we start using C++14 at the master branch?"
https://discourse.itk.org/t/may-we-start-using-c-14-at-the-master-branch/4172/23?u=niels_dekker
dzenanz pushed a commit to InsightSoftwareConsortium/ITK that referenced this pull request Jun 16, 2021
Following commit InsightSoftwareConsortium/ITKSoftwareGuide@f65f786
"ENH: Update Xcode Apple Clang compiler for nightly dashboard"

The new version numbers are based on feedback from Sean McBride, at:

Pull request "ENH: Update compiler versions for C++14"
InsightSoftwareConsortium/ITKSoftwareGuide#156

Discussion topic "May we start using C++14 at the master branch?"
https://discourse.itk.org/t/may-we-start-using-c-14-at-the-master-branch/4172/23?u=niels_dekker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants