[libcxx-commits] [PATCH] D133249: [libc++] Documents details of the pre-commit CI.

Tue Sep 6 11:46:32 PDT 2022

aaron.ballman added a comment.

Thank you for working on this!

================
Comment at: clang/www/hacking.html:33
         <li><a href="#testingCommands">Testing on the Command Line</a></li>
+        <li><a href="#testingLibcxx">Testing changes affecting libcxx</a></li>
       </ul>
----------------
Similar capitalization is used in the file.

================
Comment at: clang/www/hacking.html:292
+  <a href="https://buildkite.com/llvm-project/libcxx-ci">pre-commit CI</a>.
+  This can simply be done by adding a dummy file in the libcxx directory before
+  submitting or updating a patch in Phabricator. This change in the libxx
----------------

================
Comment at: clang/www/hacking.html:293
+  This can simply be done by adding a dummy file in the libcxx directory before
+  submitting or updating a patch in Phabricator. This change in the libxx
+  directory will cause the update of the diff to start a CI run. This dummy
----------------

================
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>
+
----------------
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.)

================
Comment at: clang/www/hacking.html:300
+  CI lacks the resources to build every Clang diff. A single run takes about
+  one hour. Secondly most changes of Clang won't affect libc++.</a>
+
----------------
philnik wrote:
> Having a secondly without a firstly seems weird.
+1 to the "secondly" being a bit weird, but also pushing back a bit on the assertion that most changes in Clang won't affect libc++: we change the output of Clang's diagnostics on an almost-daily basis.

I think it's honest to say: "It is difficult to determine which changes in Clang will affect libc++ and running the precommit CI in all circumstances is prohibitively resource intensive given how actively the project is updated." or something along those lines.

================
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>
+
----------------
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?

================
Comment at: libcxx/docs/Contributing.rst:87

-* C++20 for the Linux platform.
-* MacOS C++20 for the Apple platform.
+* C++XX for the Linux platform (where XX is the latest C++ version).
+* MacOS X86_64 and MacOS arm64 for the Apple platform.
----------------
philnik wrote:
> Complete nit, but `XX` reads to me like the digits have to be the same. I'd suggest `XY` to make it obvious that they can be different.
Or you could go with something along the lines of `C++<version>`.

================
Comment at: libcxx/docs/Contributing.rst:107
+
+The CI tests libc++ for all :ref:`supported platforms <SupportedPlatforms>`.
+The build is started for every diff uploaded. A complete CI run takes
----------------
philnik wrote:
> Not really currently. We still claim FreeBSD support without a CI runner.
Also, do I remember correctly that Windows testing is not done on a CI runner for libc++?

================
Comment at: libcxx/docs/Contributing.rst:114
+
+Typically the libc++ jobs use a Ubuntu Docker image. This image contains
+recent `nightly builds <https://apt.llvm.org>`__ of all supported versions of
----------------

================
Comment at: libcxx/docs/Contributing.rst:119-121
+Unless specified otherwise the ``main`` version of Clang is used.
+
+Unless specified otherwise the tests are executed for the latest C++
----------------
mstorsjo wrote:
> Mordante wrote:
> > mstorsjo wrote:
> > > I don't understand this paragraph - each CI run is run through the configured set of supported Clang versions - not only `main`? Or does this talk about specifics about manually running tests with the Docker image?
> > This is the version of Clang in the main branch. How about:
> > `Unless specified otherwise the nightly build of Clang from the ``main`` branch is used.`
> I still don't quite understand what this tries to say. "Unless specified otherwise" - where would I specify a different preference, when I upload a patch and it gets run through CI?
> 
> There's three different concepts involved here:
> - The CI build matrix (buildkite-pipeline.yml) which specifies a bunch of different versions of tools and standards to run the tests on. Here you can't specify a different preference - all of them are run (unless you edit the file in your patch).
> - The Docker images, where the default `clang` executable is a recent nightly build, but older releases are available as `clang-<version>`
> - The `run-buildbot` script, which just uses whatever compiler is set as default (via e.g. the `CXX` env var).
> 
> So, what about these three concepts is this paragraph trying to say - the second or the third point? This really needs to specify what it talks about - build matrix, docker images or run-buildbot script.
> 
> The same thing goes for the following paragraph.
(mostly trying to get rid of the duplicate "Unless specified otherwise".)

================
Comment at: libcxx/docs/Contributing.rst:186
+All files of the CI infrastructure are in the directory ``libcxx/utils/ci``.
+Note that quite a bit of this infrastructure is heavily Linux focussed. This is
+the platform used by most of libc++'s Buildkite runners and developers. Patches
----------------

================
Comment at: libcxx/docs/Contributing.rst:193-194
+
+Contains the Docker image for the Ubuntu CI. Since the same Docker image is
+used for the ``main`` and ``release`` branch it should contain no hard-coded
+versions [#]_.  It contains the used versions of Clang, various clang-tools,
----------------

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133249/new/

https://reviews.llvm.org/D133249