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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
