-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[cxxmodules] Add a module for experimental/string_view #12276
Conversation
Starting build on |
Build failed on ROOT-ubuntu2004/python3. Errors:
And 25 more |
Build failed on ROOT-performance-centos8-multicore/cxx17. Errors:
And 24 more |
Build failed on ROOT-debian10-i386/soversion. Errors:
And 25 more |
cbeb761
to
4a2920b
Compare
Starting build 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.
Could enhance the commit log by copy/pasting/summarizing the error that it fixes? The log will be long gone when we next look at that commit log. Thanks.
The commit message seems pretty clear to me. Can you suggest something? |
@pcanal proposes to add the literal diagnostic, which helps understand "recent failure" in two years, and which allows to search if we run into this again, e.g. in 6-26:
|
(And btw it's GCC 12, I'm not remembering seeing this issue with GCC 11 as the commit log says?) |
Nope, the problem being fixed is seen for gcc11 as the commit message points out: https://lcgapp-services.cern.ch/root-jenkins/view/ROOT%20Nightly/job/root-nightly-master/LABEL=ROOT-centos9,SPEC=noimt,V=master/3429/console |
The commit message says exactly that. I don't think I want to paste the error to say actually the same thing as the commit message says. |
4a2920b
to
a1a1804
Compare
Starting build on |
I think I see where the confusion comes from, now. I added the PR title in the commit message. |
Yes, better than the "That should fix a recent nightly failure with gcc11" that Philippe commented on, thanks for improving it! Compared to just including the actual diagnostic (something Philippe and I generally try to do these days, something that has proven super helpful several times), your current log:
does not call out
In general we tell our users (+/- always) "can we please see the actual diagnostic", and for the same reasons it's super helpful to include them here. The time we discussed this is waaay larger than the time it would have taken you to update the log to what Philippe proposes, rather than updating it to something that tries to address the issues (but seemingly fails?) But it's Philippe's review, I let him comment / deal with this if you still prefer to keep your current commit log. I simply wanted to add an explanation why Philippe suggests the inclusion of the actual diagnostic (and why I found it convincing and do it ever since he propose it to me). |
This issue exists on gcc11 and gcc12 (and I guess any gcc that made changes to libstdc++ and the experimental/string_view header file). I am not sure if pasting a diagnostic from a random system makes this clearer in any form. I agree with you this should have be 1 min review fixing an important failure that we introduced with the c++20 support and some of the new releases of maybe libstdc++. @pcanal I find pasting errors in the logs a bad practice, especially when it obfuscates the real fix. Can you suggest a commit message which adds enough information which makes me happy as well? In general it is a bit unfortunate how the whole review went for this PR. This should been just a simple "approve" as I thought we needed a quick fix which I developed in between travels and a business trip... I suspect this would help me define urgent/important in similar circumstances in future... |
@vgvassilev Let me clarify the disconnect. The title says "Add a module for experimental/string_view" That should fix a recent nightly failure with gcc11 avoiding to require The code says:
The commit content as-is seems completely unrelated to the commit log as far as I could tell (without doing research on the relationship between So I pondered whether the fix was the right fix for a problem I did not know anything about ... The right thing to do would have probably be have been to request a complete explanation of what the original problem was, what was the mechanism leading to the error and why this solution was the best solution. This was obviously much more than this seemingly simple fix required. So instead I thought to ask for a very low cost, straight forward solution: simply copy/pasting the error with no additional effort to explain in detail.
That requires to paraphrase the error and add a few more details.
Actually, I still don't know why adding |
Now that’s something I was looking for! Thank you! I think that’s a mistake in this PR. I failed to update the relevant header as well. The problem is more subtle probably as since some update of gcc it started picking up experimental/string_view which in turn somehow uses the headers only available in c++14 onwards. So perhaps we should re-export string_view… I can look at that tomorrow. |
a1a1804
to
8097eac
Compare
Starting build on |
8097eac
to
f945434
Compare
Starting build on |
@pcanal, please check the commit log, I think that's good to go. |
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.
Looks sufficient even-though the wording still leaves me a bit confused.
it needs to include "bits/ranges_base.h" which is a c++14 header
Then why is the error message talking about require C++20 context?
breaks the compilation in case of c++11.
Didn't Axel report that it failed in C++14 and C++17?
Starting build on |
Starting build on |
1 similar comment
Starting build on |
Build failed on ROOT-ubuntu18.04/nortcxxmod. Failing tests: |
Build failed on mac12/noimt. Failing tests: |
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests: |
Build failed on ROOT-debian10-i386/soversion. Failing tests: |
Build failed on mac11/cxx14. Failing tests: |
Recent gcc updates somehow make experimental/string_view available through module string_view. Then it wrongly decides it needs to include "bits/ranges_base.h" which is a c++14 header and breaks the compilation in case of c++11. This patch adds a proper experimental/string_view to disallow such shadowing.
It is not present on some versions of libstdc++ which makes the compilation of Core.pcm fail.
ae9c66a
to
e4122e8
Compare
Starting build on |
Build failed on ROOT-debian10-i386/soversion. Failing tests: |
Build failed on ROOT-ubuntu18.04/nortcxxmod. Failing tests: |
Build failed on mac12/noimt. Failing tests: |
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests: |
Build failed on mac11/cxx14. Failing tests: |
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
@phsft-bot build! |
Starting build on |
Build failed on ROOT-debian10-i386/soversion. Failing tests: |
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests: |
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
Build failed on ROOT-ubuntu18.04/nortcxxmod. Failing tests: |
Build failed on mac12/noimt. Failing tests: |
Build failed on mac11/cxx14. Failing tests: |
The failures seem unrelated. Let's merge this PR. |
That should fix a recent nightly failure with gcc11