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.
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
C++11 build errors #235
Comments
I've set up a docker with compiler in question, and am in the process of building dcmqi. I will report later today on the result. |
Just checked: most recent 0.x.y jsoncpp unfortunately shows the same errors. |
Another info: most recent jsoncpp also builds with |
@nolden I confirm I can also reproduce the compile errors with the docker I configured. Let me try the suggestion you made, and test if it passess CircleCI. |
But wait, this is not a universal flag, is it? We should make sure it builds on all 3 operating systems, including Visual Studio version that is used by Slicer dashboard. We don't even have a way to test that specific Visual Studio outside the dashboard. |
No, but it's a strong indication that there are no mandatory C++11 features in jsoncpp. Instead of setting the flag directly you could also use CMake's cross-platform features for selecting the C++ standard version, again rather on the superbuild level above dcmqi |
@nolden yes, I agree with you. I am just not familiar with how to select standard C++ in VC, and whether it is version-specific. I will look into that. |
reported in jsoncpp project: open-source-parsers/jsoncpp#600 |
Thanks for reporting there. Seems to me a switch to a newer version would be the easiest to maintain if that still builds on older compilers with C++ 0x. You can use CMake to set this without knowing about all the different flags: https://cmake.org/cmake/help/v3.1/variable/CMAKE_CXX_STANDARD.html#variable:CMAKE_CXX_STANDARD |
@nolden thank you for the hints! I am experimenting with this cmake feature in this branch: https://github.com/fedorov/dcmqi/tree/add-cxx-standard-property using this docker: https://github.com/fedorov/dcmqi-docker-tests/blob/master/Dockerfile.gcc63. At the moment, stumbled how to propagate CMAKE_CXX_STANDARD to the dcmqi target. Independently of the jsoncpp update/changes, we need to set std++98 for all targets, since std++14 is default for gcc 6: https://gcc.gnu.org/gcc-6/changes.html. |
@nolden I could not figure out how to set CMAKE_CXX_STANDARD variable - the flags are not set for the compiler. When I manually set
|
Unable to compile plain ITKv4 (the same hash as is used by Slicer https://github.com/Slicer/Slicer/blob/master/SuperBuild/External_ITKv4.cmake#L45) with gcc 6.3 and with @thewtex do you know if this is expected to work?
|
@fedorov We do not have a dashboard build for that specific configuration, but I expect that it would work. Make sure that
or
|
@thewtex thank you, I confirm I can compile ITK with gcc 6.3 using this approach. Back to dcmqi issue, still have troubles related to C++11, even when setting
|
Oh, this looks like an CMake / ITK problem (Gitlab, Mantis) we have also encountered in MITK, thought it was fixed but at first glance it looks like the same problem, we worked around it by specifying the flags directly for sub-projects which are affected (like ITK). @thewtex : do you know anything about this? The (non-working) solution using CXX_STANDARD looks like this: nolden@961cc82 @fedorov : does Slicer superbuild work with gcc 6.3 ? If we can sort this issue out, would you agree to update jsoncpp to a more recent version? |
@fedorov : actually I should rephrase my last question. Updating jsoncpp should fix the problems with gcc 6.3 and it should still build with C++98 , independent of the ITK/CMake problems. But what was the original error the CircleCI compilers had with the newer jsoncpp? I think this was the rationale behind downgrading it. |
I don't know, and I don't have a good way to test this. I was doing dcmqi gcc 6.3 tests using this Dockerfile. I guess I could set one up for Slicer too.
Let's review the facts:
So I think the proper solution to the problem you experience is not update to 1.x.y, but fix the issue with incorrect propagation of flags so that C++98 is used with gcc 6.3. Do you agree?
Well, that is the hypothesis to be tested on all 3 platforms that are used to build Slicer ... and even if it works, how can we say it will stay this way, since jsoncpp explicitly says C++11 is needed for 1.x.y?
It was indeed related to C++11: https://circleci.com/gh/QIICR/dcmqi/92
With all that said, we could instead just switch to rapidjson:
This would probably be my preferred solution, because alternative is debugging cmake/ITK/etc, which is close to intractable for me, and gives no long term benefit. Moving to rapidjson just seems to make sense. What do you think? |
I know there have been many fixes in recent CMake for propagating this type of information -- it is worth trying the build with the latest release of CMake. |
I was just asking, since if Slicer aims at being compatible with gcc 6.3, it would also need a solution to properly enforce older C++ standard in all projects.
Yes, but it also builds with C++98, see above
I think there is a difference between using C++11 or being buildable with C++11 features enabled. In general the language is backwards compatible. For the MSVC compilers there is even no option to set C++98, you have to switch back to an older cl.exe to use C++98 in say MSVC 2015.
Not fully, sorry. Propagating the flags would not solve the issue, since I want to compile MITK with C++11 (actually C++14) flags and the general recommendation is to keep this setting consistent through your whole build.
We could ensure this by just not updating 1.x.y anymore, like it's not updated at the moment. For the Slicer platforms I cannot estimate how difficult this will be to test.
Ok, this was an older version of 1.x.y . In the current one that worked for me they improved the version detection a lot, JSON_CPP_OVERRIDE seems to be intended to distinguish this but it had a bug ...
Looks very nice and active for me. Any idea of a timeframe? Would it be ok if I create a pull request for dcmqi which allows for using jsoncpp 1.x.y as an option? Could be easier than patching the whole file back on the MITK side and it would also allow others to make use of it if necessary. |
@nolden now that you clarify that you need it to build with C++11 (14!) support, your argument makes sense to me.
It is not the first priority. I need to finish refactoring of the code before taking on any new features.
Would you mind making a PR with the update? Let's see if it works on CIs, and then on Slicer dashboard, and if all goes well, we might just upgrade it, and later switch to rapidjson.
If this will be set at compile time, and won't break 0.x.y option - definitely, your contribution is welcome! |
Ok, just to make sure I understand you correctly: could you test this for Slicer? Or should I go for the second option, making it optional? |
@thewtex Thanks for the hint! I just tested this with most recent CMake, unfortunately it shows the same errors. I think I have to create a minimal non-working example for the bug report. |
@nolden I don't have the capacity to test it for Slicer. I don't even have a Slicer developer environment set up for Windows (it requires an old version of Visual Studio, which I don't want to install on the only Windows box that I have, since it might mess up my existing recent Visual Studio installation). What I suggested to do is 1) make a PR with the update - this will test dcmqi on all platforms; 2) if tests pass, take a risk and integrate, and take a look at Slicer dashboards if this caused any issues. Making it optional would be preferred. If we see problems on Slicer dashboard, it will be easy to quickly revert to using 0.x.y for Slicer and still keep 1.x.y for MITK, instead of reverting commit. |
Closing. Slicer, DCMTK and DCMQI can now all be built with C++11 enabled |
As reported by @nolden: "Currently it doesn't build with recent compilers, e.g. gcc 6.3 on Fedora"
The text was updated successfully, but these errors were encountered: