Mordante marked 24 inline comments as done. Mordante added inline comments.
================ Comment at: clang/www/hacking.html:295-296 + directory will cause the update of the diff to start a CI run. This dummy + file will also add the libc++ group to the list of reviewers. The status of + the build will be available in Phabricator.</p> + ---------------- zero9178 wrote: > tahonermann wrote: > > ldionne wrote: > > > Mordante wrote: > > > > philnik wrote: > > > > > aaron.ballman wrote: > > > > > > I am guessing the dummy file should then be removed before landing > > > > > > the commit and we should document that? > > > > > > > > > > > > (FWIW, adding a dummy file feels really unclean as a design -- it's > > > > > > mysterious behavior for new contributors, which the documentation > > > > > > helps with a bit, but also it's a risk for checking in files that > > > > > > are never intended to be in the tree. If there's a way to improve > > > > > > this somehow so we don't need to trick the precommit CI into > > > > > > running, that would be really nice and totally outside of the scope > > > > > > of this review.) > > > > > It would be great if just the bootstrapping build could be run on all > > > > > clang reviews. That should show most problems with changes in clang. > > > > > If there are any problems in libc++, fixing them would result in a > > > > > full CI run. Having libc++ run against the patch would probably also > > > > > be awesome as a smoke test for any clang changes. > > > > +1 to what @ldionne said. > > > > > > > > One of the advantages would be that it's possible to test Clang against > > > > the libc++ code base using different language versions, by defining > > > > multiple test runs using different language versions. (Slightly related > > > > to > > > > https://discourse.llvm.org/t/lit-run-a-run-line-multiple-times-with-different-replacements/64932) > > > > @aaron.ballman is looking at extending the Clang pre-commit CI > > > > something worth looking into? > > > > > > > > > > > I agree. Let's: > > > > > > 1. Setup a `buildkite-pipeline.yml` file that controls the jobs done for > > > Clang. I think right now this is hardcoded somewhere in > > > https://github.com/google/llvm-premerge-checks > > > 2. Simply add `LLVM_ENABLE_RUNTIMES=libcxx` to the config that Clang uses > > > so they'll automatically build libc++ with the Clang that was just built > > > anyway. That would run on Clang's own agents that are used for Clang > > > tests, not on the libc++ CI agents. We can adjust if running the libc++ > > > tests causes too much strain on the infrastructure running Clang CI. > > > > > > That way, a basic bootstrapping build of libc++ will be run on every > > > Clang change, and TBH I think all of this added documentation for Clang > > > folks to test libc++ changes can simply go away. There really wouldn't be > > > a reason to ever add a `.DELETE_ME` file to libc++ for a Clang patch > > > anymore. > > This sounds like a good approach to me. This might be a bit tangential, but > > if `libcxx` is present in `LLVM_ENABLE_RUNTIMES`, could we also have the > > `check-all` target depend on `check-libcxx`? That would help ensure that > > developers run libc++ tests locally before they hit the CI infrastructure. > > Likewise for `compiler-rt` and `libcxxabi` and the `check-runtimes` target. > This is currently being implemented by https://reviews.llvm.org/D132438 and > landed before but had to be reverted unfortunately. I removed the part of the dummy file. Having a `check-all` indeed sounds very nice to have. ================ Comment at: clang/www/hacking.html:311-312 + <p>Unlike Clang, libc++ supports multiple versions of Clang. Therefore when a + patch changes the diagnostics it might be required to use a regex in the + "expected" tests to make it pass the CI.</p> + ---------------- Mordante wrote: > philnik wrote: > > aaron.ballman wrote: > > > Should we document the expectation that breaking libc++ due to conforming > > > changes in Clang (in terms of diagnostics and bug fixes, not so much in > > > terms of introducing new language extensions) are generally the > > > responsibility of libc++ maintainers to address, but Clang contributors > > > should attempt to reduce disruptions as much as possible by collaborating > > > with libc++ maintainers when this situation comes up? > > That's definitely a good idea. Maybe mention that clang contributors should > > ping the #libc group to get the attention of libc++ contributors so we can > > prepare a follow-up patch or, if the author is comfortable with it, also > > fix libc++ in the same patch (and hopefully get fast approval). Most > > breakages should be relatively simple to fix. > I like the idea to collaborate more. > > I want to give this some thought and especially how to word it. I want to > avoid that it's seen as a carte blanche to commit Clang changes which break > libc++. This would mean HEAD is broken and that's a huge issue for downstream > users like Google who tend to follow HEAD closely. > > I would prefer to use commandeering and instead of two patches where one > leave HEAD in a broken state. Even if it's short it would make bi-secting > harder. Personally I really dislike the idea of having commits that knowingly > leave a repository in a broken state. It also doesn't align with the LLVM > developer policy > https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy > ``` > As a community, we strongly value having the tip of tree in a good state > while allowing rapid iterative development. > ``` > > Should we document the expectation that breaking libc++ due to conforming > changes in Clang (in terms of diagnostics and bug fixes, not so much in terms > of introducing new language extensions) are generally the responsibility of > libc++ maintainers to address, but Clang contributors should attempt to > reduce disruptions as much as possible by collaborating with libc++ > maintainers when this situation comes up? @aaron.ballman I have given this some thought. I think it would be best to discuss this with some libc++ and Clang contributors/code owners. For libc++ at least @ldionne should be present since he's the libc++ code owner. I would like to be present too. After we have agreement we can update this documentation accordingly. ================ Comment at: clang/www/hacking.html:279 + + <p>Some changes in Clang affect <a href="https://libcxx.llvm.org";>libc++</a>, + for example:</p> ---------------- aaron.ballman wrote: > ldionne wrote: > > This follows my previous comment, but we really don't want to encourage > > Clang folks to start using a `.DELETE_ME` file for testing Clang changes. > > Indeed, roughly 54/55 of our CI jobs don't use the just-built Clang. The > > only relevant CI job for a Clang change is the Bootstrapping build. As a > > result, using a `.DELETE_ME` file is extremely wasteful of libc++ CI, > > since almost all the CI jobs are completely irrelevant to the change being > > tested. Furthermore, it will also provide a false sense of security for > > Clang folks, who will think "hey but I ran the CI under all > > configurations", not understanding the details. > > > > So I think we need to set up libc++ testing for Clang changes, but we > > should discourage folks from running the libc++ CI itself as a poor proxy > > for testing Clang changes. Most of the jobs are just not useful at all from > > the perspective of testing a Clang change. > > > > So I'd rather not add this part of the documentation at all, to avoid > > creating a problem for ourselves. > > So I'd rather not add this part of the documentation at all, to avoid > > creating a problem for ourselves. > > FWIW, I love the idea of setting up libc++ testing for Clang changes so we > don't need a .DELETE_ME file. I think that's ultimately where we all would > love to land. I'm fine either pulling these docs in anticipation of that work > or leaving the docs here until that work is done. Since @ldionne strongly prefers to remove it I'll remove it. Not in the scope of this review, but let's investigate what is needed to get this setup libc++ in the Clang pre-commit CI. Having it in Clang's pre-commit CI will make it easier to test multiple configurations instead of only what's available in libc++'s bootstrapping build. I have experience with the libc++ pre-commit CI so maybe I can help with the Clang version. Part of the documentation is intended for Clang developers to be aware of libc++'s pre-commit CI. When Clang tests libc++ in their pre-commit CI they might do minor libc++ changes. These changes will trigger the libc++ precommit CI. So that part should still be documented. @ldionne so please have a look after the next update. Note most of the changed documentation makes really sense when Clang tests libc++. If it's not feasible to realize in short term we might want to comment out the new section. But I hope we can get this working rather soon. ================ Comment at: libcxx/docs/Contributing.rst:182-183 +Note that quite a bit of this infrastructure is heavily Linux focused. This is +the platform used by most of libc++'s Buildkite runners and developers. Patches +for other platforms are welcome. + ---------------- ldionne wrote: > IMO this would have its place in the part of the documentation where we list > all the platforms that we support. We *are* interested in patches for adding > support to new platforms. However, I'm not sure what patches for porting our > CI infra to other platforms would mean, since e.g. the Docker image is > running Linux by definition. Same for the macOS bits of our CI infra -- it > wouldn't make sense to try to port that to other platforms. I've applied your suggestion and removed the Patches part. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133249/new/ https://reviews.llvm.org/D133249 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
