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

Tue Sep 13 09:59:12 PDT 2022

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM w/ comments. Thanks a lot for writing this!

================
Comment at: clang/www/hacking.html:279
+
+  <p>Some changes in Clang affect <a href="https://libcxx.llvm.org">libc++</a>,
+  for example:</p>
----------------
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.

================
Comment at: libcxx/docs/Contributing.rst:91-92
+
+.. comments The new part got longer than expected, would it make sense to move
+   it to its own page?
+
----------------
I think this is fine in this file for now. We can reorganize the docs if needed with a fresh holistic view if needed.

================
Comment at: libcxx/docs/Contributing.rst:103-104
+CI is hosted on `Buildkite <https://buildkite.com/llvm-project/libcxx-ci>`__ and
+the build results are visible in the review on Phabricator. Before committing a
+patch please make sure the CI is green.
+
----------------

================
Comment at: libcxx/docs/Contributing.rst:125-126
+
+.. note:: Updating the Clang nightly builds in the Docker image is a manual
+   process and is done at an irregular interval. When you need to have the
+   latest nightly build to test recent Clang changes best ask at the
----------------

================
Comment at: libcxx/docs/Contributing.rst:127-129
+   latest nightly build to test recent Clang changes best ask at the
+   ``#libcxx`` channel on
+   `LLVM's Discord server <https://discord.gg/jzUbyP26tQ>`__.
----------------

================
Comment at: libcxx/docs/Contributing.rst:131-133
+.. comments ``irregular`` is used on purpose. Committing to a schedule seems
+   unwanted; people might be unavailable to update the image, the nightly
+   builds fail and are not uploaded for multiple days (this is not uncommon).
----------------
This can be removed.

================
Comment at: libcxx/docs/Contributing.rst:157-158
+  uses that Clang version to build and test libc++. This validates the current
+  Clang and lib++ are compatible. When making changes in Clang that affect
+  libc++ this is the build to validate all is well.
+
----------------

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

================
Comment at: libcxx/docs/Contributing.rst:196-198
+.. [#] Currently it contains hard-coded versions using symlinks. They will be
+   removed after LLVM 16 has branched. Removing them before that time risks
+   breaking the CI of the release branch.
----------------
I would remove this comment, since it's going to become irrelevant quite soon, and I think we're likely to forget to update it. There are already comments in our Dockerfile to that effect.

================
Comment at: libcxx/docs/Contributing.rst:204
+Helper script that pulls and runs the Docker image. This image mounts the LLVM
+monorepository at ``/llvm``. This can be used to test with compilers not
+available on your system.
----------------

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133249