dblaikie added a comment. In D139986#4017650 <https://reviews.llvm.org/D139986#4017650>, @Mordante wrote:
> In D139986#4017467 <https://reviews.llvm.org/D139986#4017467>, @dblaikie > wrote: > >> In D139986#4008169 <https://reviews.llvm.org/D139986#4008169>, @Mordante >> wrote: >> >>> In D139986#4005997 <https://reviews.llvm.org/D139986#4005997>, @dblaikie >>> wrote: >>> >>>> In D139986#4003873 <https://reviews.llvm.org/D139986#4003873>, @Mordante >>>> wrote: >>>> >>>>> In D139986#4001180 <https://reviews.llvm.org/D139986#4001180>, >>>>> @Michael137 wrote: >>>>> >>>>>> Missed couple of test cases in libcxx >>>>>> About to fix those >>>>> >>>>> There were more breakage due to this patch, which I fixed in D140272 >>>>> <https://reviews.llvm.org/D140272>. >>>>> >>>>> Next time please don't commit patches when the pre-commit CI is red. This >>>>> build https://buildkite.com/llvm-project/libcxx-ci/builds/16118 shows the >>>>> patch will break libc++. These pre-commit CI jobs were added specifically >>>>> to aid the Clang developers to validate whether their changes break >>>>> libc++. Libc++ heavily relies on its pre-commit CI so breaking the CI has >>>>> a huge impact. >> >> >> >>>>> Note that when libc++ breaks there might be other projects that use the >>>>> latest Clang HEAD that will be affected too. (Not likely with >>>>> diagnostics, but likely when the modular build fails.) >>>>> >>>>> When you have issues resolving the libc++ issues you can always reach out >>>>> to us for assistance. >>>> >>>> Perhaps these LibC++ tests shouldn't be testing/using clang warnings? >>>> (could the warnings be turned off in the libc++ tests?) >>> >>> In the failing tests we want to make sure we marked a struct as >>> `[[deprecated]]`. >> >> At least looking at >> https://reviews.llvm.org/rG54d7c4dc870c5822df3d5ce538ad3936ac6405fe - that >> could be made more robust to clang changes by perhaps using `// >> expected-warning {{deprecated}}` - unlikely that the word deprecated will be >> removed from a deprecation warning, or that we'll emit a deprecation warning >> on some unrelated line from the usage. This would make the test more >> resilient to phrasing/spelling changes without, imho, significantly reducing >> the functionality of the test. >> >> Though even more generally, I could see a test being written to use >> `-Werror` and various single uses of deprecated entities, checking that >> they're rejected - though that'd probably slow things down too much (singel >> process invocations for each deprecated entity) - so I'd say the above >> (`{{deprecated}}`) is probably a fairly good balance of making the tests >> less brittle while keeping them fast/functional. >> >> (for `locale.convenience` for instance - I think that test could be >> simplified to name each type separately, rather than using them together - >> so they're on separate lines. Or maybe you can separate them onto different >> lines despite the nesting, splitting the outer templtae name over multiple >> lines) > > I don't see how that helps to validate whether we have the expected error or > an error due to ill-formed code. For example just forgetting a semicolon. reducing the wording checked for wouldn't have that problem - but yeah, going to totally wording agnostic testing wouldbe trickier. It could still avoid the "missing semicolon" problem by compiling with/without -Wdeprecated - expecting success without it, and failure with it. But I suspect the process launching overhead wouldn't be ideal for that direction anyway. At least reducing the dependence on specific warning wording/spelling would be help make the testing less brittle. >>> In other tests we validate error messages to make sure things are >>> ill-formed as mandated. We test the diagnostic to make sure the compilation >>> fails for the expected reason and not a different compilation error. Do you >>> have a suggestion how we can do that without checking Clang diagnostics? >>> >>> In general I consider it bad practice to commit code when the CI is red. >> >> My take on this is that, at least to my understanding, libc++ implemented >> this CI without discussiong/buy-in from other LLVM subprojects that libc++ >> depends on - that I'm not sure were/are willing to accept this extra >> constraint on their development. It's not suitable to bring up a CI in that >> way and then say "make sure you don't break this" - we didn't agree not to >> break it. Especially with tests like these that could/should be written >> differently to be less brittle. Perhaps I've misunderstood this & there is >> more explicit buy-in from Clang developers/owners about this relationship >> between the two subprojects? >> >> Generally prior to this it was up to libc++ folks to cleanup after upstream >> project's breakage (like lldb still does, to some extent) - though >> buildbots, etc, do provide llvm/clang contributors some chance to >> proactively provide fixes. > > This CI has been discussed with members of the Clang community. In the past > I've spoken with @aaron.ballman. I know @erichkeane requested @ldionne to > create this CI recently. (I wasn't present during this meeting.) I don't know > whether or not the creation of this CI has been further communicated in the > Clang community. Note that this CI is a benefit for Clang too. Some patches > that break libc++, also break code in other external projects. This patch > probably doesn't break external projects, but @erichkeane's D126907 > <https://reviews.llvm.org/D126907> was reverted several times due to breakage > in external projects. I also discovered some issues with that patch in > libc++, so there having libc++ as test codebase would have been helpful. *nod* it still seems a bit unreliable, though (like doesn't report existing failures differently from new failures, maybe? Producing false positive failures), which produces some tensions. > I'm not asking Clang developers to fix libc++. But I ask not to commit code > which is known to break other subprojects without giving that subproject a > headsup. Ah, perhaps there's some way to automate that then? So it doesn't involve humans - the precommit CI infrastructure could email libc++ developers about the failure directly, rather than requiring the human developer to be the message carrier? > In D139986#4017468 <https://reviews.llvm.org/D139986#4017468>, @dblaikie > wrote: > >> Oh, and I meant to mention >> https://buildkite.com/llvm-project/libcxx-ci/builds/16118#01851abb-f13e-4977-91df-b4274402b65a >> doesn't seem, at least to my cursory reading, to show diagnostic checking >> failures, but crashes? Is there something broken in the UI that's meant to >> show the diagnostic mismatches you ended up fixing? (or is the link out of >> date at this point, and the original failures are no longer visible? or >> something else?) > > When I look at that output for C++2b I see > > error: 'warning' diagnostics expected but not seen: > > File > /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp > Line 19: 'codecvt_utf8<wchar_t, 1114111, 0>' is deprecated > > File > /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp > Line 20: 'codecvt_utf16<wchar_t, 1114111, 0>' is deprecated > > File > /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp > Line 21: 'codecvt_utf8_utf16<wchar_t, 1114111, 0>' is deprecated > > error: 'warning' diagnostics seen but not expected: > > File > /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp > Line 19: 'codecvt_utf8<wchar_t>' is deprecated > > File > /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp > Line 20: 'codecvt_utf16<wchar_t>' is deprecated > > File > /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp > Line 21: 'codecvt_utf8_utf16<wchar_t>' is deprecated > > For the modular build I also see ICEs, which are compiler bugs. The modular build ICE is something pre-existing/not caused by this patch, right? Makes it hard to know when there are real failures if there are false positives like this. And at least for me, when I go to that link and click on the "C++2b" bar I only see the modular crash, I don't see these deprecated diagnostic errors: https://imgur.com/a/ZJdbumR And looking at the raw log <https://buildkite-cloud.s3.amazonaws.com/logs-by-pipeline/3f135a9c-106c-4a9e-a748-96d79d4149c3/01851abb-10d2-45ff-b442-5760a020df68/01851abb-f13e-4977-91df-b4274402b65a.log?response-content-disposition=inline&response-content-type=text%2Fplain&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ASIAQPCP3C7LQG3TTADG%2F20221228%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20221228T141745Z&X-Amz-Expires=600&X-Amz-Security-Token=IQoJb3JpZ2luX2VjEPr%2F%2F%2F%2F%2F%2F%2F%2F%2F%2FwEaCXVzLWVhc3QtMSJHMEUCIQCzshO1Yu8j%2BU37DDwBeT%2FTo3mOMcMk0Yt%2FPyNDz8fmswIgIGr%2BfNhLi64pvo6hhRqPmDd4Bj8AcFEm4O2fbPXAdy8qzAQIQxAAGgwwMzIzNzk3MDUzMDMiDLCte2%2B4I9d40AN7kiqpBNNLj4TZamjuYd07H1pu1VyBSckiIccc0NOi8XEBSyoHPfUGVLof5bHGmkumO6rg4D%2FHCfIk3W2z4viftW29tLEjGGIbMw5h9jvNUepPOqUPLzCjNshuESn6nutL4w9vUxRL8zNpwNHsr%2B%2FtbGaZbwev6d55Hi%2F7PHTVwVusKp60el5zFlI3VAVB2Pnc6sax3AQsf1TOWcMcDIArlCRAkvjecAlpT%2BUVTbzKG1ai0KwiyZbrThurkI3qySr0%2Fn8f5Vkli8%2F4vFsuQTV6L586b%2F7gnw7w7Am4zw2ieqD5UZTXAY3PdR1ZPzbZ6bksTGU0Ui3O0xjifaaSqXAjkeflIun2a7a9Zx%2B8ru0wzhXVyA4XbiUVLgSutIFVPAFDpqbZLy9sD8wu3GkwTeczWWG%2FIG8EdCcG64GNckzTHCN6tBxV9DqG7H7%2Bm%2BfN%2B9aVHE25yOy7i6TvRRpm5uNUw0YP%2FjM9P4ipwbW3KCTmQOxkdr4wPZNG%2FP8KLnc0JexpMCPKQ0j%2FtUBiYut63PibAdvcziJmKGV6SnX%2Bd1S9PUQfqDYg4PwKgliljtF%2Fgupzf2Mh6oh37GkDlP06OTPVct7RdoloNzcOdMubLj6WHtHIQ7AflrQVAxAiMpi3Ttj%2B7Tua%2Bue7XanvCTgYaOZa9mhZKUp2o4Xxzbp%2Be0mrSQB7lDSr42tCNDIN5qfbMMv4aa0O5A6OycyrxNaXohNQdlpEsT18oKm7DXBDZSUwoJ%2BwnQY6qQHi%2FmCpyZ3dt4ZmMwfy8eM3XBnV6ouKmK9aRkKoYSAAETfKxuZ1Yh68JprDVNozsADM%2BNlS7jEbndrr0RmmFUSKHsXJ8CiyMWDlm7G1wUBOXcxE65NhLgF3bCS87nQX5IfRipF%2FAUjrEpCr4lCrRb0U%2FN1XMfpzGXRSKb09tsQeUcO35trDZfPHIW70apPjvv%2FNKda1px6LVcrcXMyIZaZPJC2Plm4j3mQP&X-Amz-SignedHeaders=host&X-Amz-Signature=a1d2e528784595db9e52a14b8362a22d274f3b34a197e724f88606dba1adbb4c> ah, there I do see the failures if I search for them. But amongst so much other noise that I don't think it's reasonable to expect anyone to find them, really? As they're all of about 50 lines in a many thousand line log file. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139986/new/ https://reviews.llvm.org/D139986 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
