-
Notifications
You must be signed in to change notification settings - Fork 11
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 compatibility #366
base: master
Are you sure you want to change the base?
C++ 11 compatibility #366
Conversation
I agree that it is interesting that REST can be installed for a wider amount of systems and audience. |
I think we decided some time ago to move to C++17, I don't understand why we should revert to previous C++ versions, C++11 is from more than 10 years ago. Also, I don't understand the reasoning behind collaborations or colaborators. Collaborations or collaborators are not bounded to any particular C++ version, only operative systems are. If you have an OS or a server which is not compilant to C++17, you just have to upgrade it. In case someone is using an old ROOT version not compatible with C++17, I would recomend them to upgrade their ROOT version. I don't understand why we have to move backward. |
I don't think making it compatible with c++11 is moving backward. I did not revert the code. I am just adding the traditional solutions with If you are a fan of modern technology and modern coding style, it is OK. But to become public you need to make the most happy. I struggled for 3 days in my new institude, asking the IT guys and trying to compile ROOT with c++17 myself. Finally the best solution is to modify REST code. I also heard from the USTC group that they spend also over a week to get REST installed. We argued at that time and they had trouble upgrading the system. In any case these are bad experience. |
I also see no problem that we keep REST compatible with C++11 if we are reaching more people. We cannot ask ourselves about the decisions other collaborations are taking. The latest versions of ROOT keep compatibility with C++11, probably because of the same reasons we should keep compatible with a broader audience, why we would not want to be also compatible with C++11? It is a pity that there will be people that cannot benefit from REST just because we just don't want to keep compatibility with C++11. This I don't understand. And from the changes I see in this PR, it is not a major issue. |
There are not additional features in C++11, it is the other way around C++17 has additional features with respect to C++11. I can understand your frustration if you cannot install the code in a particular server in your institute, but you have to understand that the problem is not in REST-for-Physics, the problem is that IT from your institute never upgrade the server in 5 years. You can however try to install from source the different gcc and root versions localy although I think this can be also problematic. You can also try to use CVMFS that provide several packages for different OS. Also, you can ask in the forum or create issues about this, I don't know in which OS you find the issue or the errors that you found. I think that just to create a PR with these changes is not the way to go. Also you have to think in the developers that spend time trying moving to C++17 features, namely structure bindings, std::filesystem or so, will have their changes reverted. I disagree that we have to revert the code just because of lazy IT guys which doesn't like to upgrade servers. Imagine that you have to revert to Makefile because CMake is not installed in your old fancy server, do you think this would be acceptable? |
Personally I believe that the real problem is the institute policy or leadership not being aware that this requires to take care of it, so, it will never fund or push enough support for this. I haven't seen any lazy IT in my lifetime. Any IT is overloaded. I think we can afford it, but it is also true that one week work on any server to get an updated system is worth. But this is their problem, we should be more flexible if we want REST-for-Physics is more broadly used, which I believe is in our benefit and a positive impact of our work!
I have also seen many of the efforts and work I invested into REST-for-Physics reverted, smashed or obsolete. I don't think this is the case here, the code is now compatible with C++17. The solution with the
Installing |
source/framework/CMakeLists.txt
Outdated
@@ -11,6 +11,14 @@ file(GLOB_RECURSE addon_src | |||
"tiny*cpp" | |||
"startup.cpp") | |||
|
|||
if (NOT ${REST_EVE} MATCHES "ON") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be here, because if not TRestComplex
was not properly excluded. I think because the excludes variable was not transferred properly to COMPILEDIR
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not remove mpfr
libraries from the system and try -DREST_MPFR=OFF
. It will still try to compile `TRestComplex.
See also https://root-forum.cern.ch/t/question-about-how-to-cast-in-pyroot/53116/20
I 100% agree with @juanangp here. We need to draw a line at some point and I think C++11 is just too old, many current C++ projects don't support it anymore and draw the line at C++14 (which is still pretty old). We decided to set C++17 as the minimum since it provides some quality of life features and we could do it without much hassle. For example, latest root version binaries are compiled with C++17 so I think this is not too much forward-thinking.
I get your point @nkx111, supporting the most users is always good, but we have to draw the line at some point, why not go older than C++11? If some user raises the issue of not being able to install REST due to much older dependencies (C++03?) will we also modify the code in order to support it? I think that in order to be successful, a project should not only target the broadest audience, but it also should try to appeal to potential contributors, and most people don't want to be stuck using C++11. Besides, at some point we will need to migrate to a higher standard and change all the code again, why not do it now? I think this PR is very important and should only be merged once we are all on the same page. |
The only line we shall follow is what ROOT is following. As long as ROOT could be compiled with C++11, we shall keep C++11 compatibility. That would be very clear. Of course no one will raise problem of C++03, since ROOT could not be installed on that one.
I know there are plenty of good features on C++17 for us developers. For REST libraries I think we can set C++17 as the standard. But C++11 support shall still be provided in the framework. Therefore anyone already using ROOT6 could easily clone and install REST(framework) for a try, with no additional requirements. Note that we indeed don't recommend to use 14 or 11. If you look into CMake in this PR I already added warnings when enabling C++14 or 11. |
Okay I agree the C++03 argument was a bit over the top and using ROOT support of C++ standards as a guideline is in principle a good idea, since ROOT is the only hard requirement of REST. However in practise most users of REST also want to install the libraries and packages (especially Geant4), so even though Geant4 is not technically a requirement I think since most people use it we should also consider it as such, at least partially. Currently Geant4 has moved to C++17 standard and (I assume, didn't try it myself) is not possible to compile it with a lower standard. (for reference check the official installation guide and seach for |
Then we shall make restG4 support old geant4 also. We can create PRs in other repository. We must make agreement on the basic policy here.
We should not step too much forward than ROOT and Geant4. Even the latest Geant4 forces c++17, it doesn't mean that everyone is using c++17 Geant4. Even the latest ROOT version (6.26.x) is also moved to C++14 standard(still lower than the current REST requirement), not everyone is using c++17 ROOT. Why don't upgrade it? Because upgrading ROOT version also requires a re-compile for your existing code, and could cause unknown problems. See here their ABI backward compatibility. Assuming that a science project usually takes time of ~5 years, we can draw a line of REST minimum requirement to support ROOT and Geant4 versions for as long as 5 years. So after |
I still think we should retain c++11 compatibility for REST. At least for the framework. Otherwise it would be less acceptable by other collaborations. With this PR we can install REST framework under gcc 4.8.5. (Also the latest ROOT could be compiled with that gcc, why shouldn't us?)