-
Notifications
You must be signed in to change notification settings - Fork 14.6k
[libcxx] Update testing documentation about CI container images. #149192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Add information to the libcxx testing documentation, about the names of the for the new CI libcxx runner sets, their current values, and how to change the values or the runner set being used.
@llvm/pr-subscribers-libcxx Author: None (cmtice) ChangesAdd information to the libcxx testing documentation, about the names of the new CI libcxx runner sets, their current values, and how to change the values or the runner set being used. Full diff: https://github.com/llvm/llvm-project/pull/149192.diff 1 Files Affected:
diff --git a/libcxx/docs/TestingLibcxx.rst b/libcxx/docs/TestingLibcxx.rst
index 56cf4aca236f9..d278e18093e87 100644
--- a/libcxx/docs/TestingLibcxx.rst
+++ b/libcxx/docs/TestingLibcxx.rst
@@ -139,6 +139,43 @@ Also note that the Docker container shares the same filesystem as your local mac
modifying files on your local machine will also modify what the Docker container sees.
This is useful for editing source files as you're testing your code in the Docker container.
+Updating the CI testing container images
+----------------------------------------
+
+The libcxx linux premerge testing can run on one of three sets of
+runner groups. The three runner group names are
+"llvm-premerge-libcxx-runners", "llvm-premerge-libcxx-release-runners"
+or "llvm-premerge-libcxx-next-runners". Which runner set to use is
+controlled by the contents of https://github.com/llvm/llvm-project/blob/main/.github/workflows/libcxx-build-and-test.yaml . By default, it uses
+"llvm-premerge-libcxx-runners". To switch to one of the other runner
+sets, just replace all uses of "llvm-premerge-libcxx-runners" in the yaml
+file with the desired runner set.
+
+Which container image is used by these three runner sets is controlled
+and set by the variable values in
+https://github.com/llvm/llvm-zorg/blob/main/premerge/premerge_resources/variables.tf.
+The table below shows the variable names, their current values, and
+the runner sets to which they correspond.
+
++------------------------------------+---------------------------+--------------------------------------------------------------------------+
+|Runner Set |Variable |Image Value |
++====================================+===========================+==========================================================================+
+|llvm-premerge-libcxx-runners |libcxx_runner_image |ghcr.io/llvm/libcxx-linux-builder:16f046281bf1a11d344eac1bc44d11f3e50e3b5d|
++------------------------------------+---------------------------+--------------------------------------------------------------------------+
+|llvm-premerge-libcxx-release-runners|libcxx_release_runner_image|ghcr.io/llvm/libcxx-linux-builder:16f046281bf1a11d344eac1bc44d11f3e50e3b5d|
++------------------------------------+---------------------------+--------------------------------------------------------------------------+
+|llvm-premerge-libcxx-next-runners |libcxx_next_runner_image |ghcr.io/llvm/libcxx-linux-builder:16f046281bf1a11d344eac1bc44d11f3e50e3b5d|
++------------------------------------+---------------------------+--------------------------------------------------------------------------+
+
+Note: The three runner images are (temporarily) all the same, but that
+will not normally be the case.
+
+To change the container image, the variable.tf file in llvm-zorg must
+be changed. Once that change has been merged, an llvm-zorg
+administrator must use terraform to apply the change to the running
+GKE cluster.
+
+
Writing Tests
=============
|
libcxx/docs/TestingLibcxx.rst
Outdated
Which container image is used by these three runner sets is controlled | ||
and set by the variable values in | ||
https://github.com/llvm/llvm-zorg/blob/main/premerge/premerge_resources/variables.tf. | ||
The table below shows the variable names, their current values, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to have the current values here or refer the reader to the SoT (variables.tf
in llvm-zorg
?). Having the duplication, especially across repos I think is going to mean the docs are frequently out of date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we should strive to have a single source of truth (and have that source of truth be in the monorepo).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if we only want a single source of truth, though, it's going to have to be in llvm-zorg. We can put a link to it in the monorepo.
libcxx/docs/TestingLibcxx.rst
Outdated
Note: The three runner images are (temporarily) all the same, but that | ||
will not normally be the case. | ||
|
||
To change the container image, the variable.tf file in llvm-zorg must |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want instructions here on how to modify libcxx/utils/ci/docker-compose.yml
(maybe post #148073) to bump the container version along with how the runner binary can be bumped separately.
libcxx/docs/TestingLibcxx.rst
Outdated
@@ -139,6 +139,43 @@ Also note that the Docker container shares the same filesystem as your local mac | |||
modifying files on your local machine will also modify what the Docker container sees. | |||
This is useful for editing source files as you're testing your code in the Docker container. | |||
|
|||
Updating the CI testing container images |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the documentation updates! I think this would fit better if integrated into https://github.com/llvm/llvm-project/blob/main/libcxx/docs/Contributing.rst#pre-commit-ci, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
libcxx/docs/TestingLibcxx.rst
Outdated
Which container image is used by these three runner sets is controlled | ||
and set by the variable values in | ||
https://github.com/llvm/llvm-zorg/blob/main/premerge/premerge_resources/variables.tf. | ||
The table below shows the variable names, their current values, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we should strive to have a single source of truth (and have that source of truth be in the monorepo).
Also update & correct some of the documentation.
I apologize for the long delay. I think this is ready to review again. |
libcxx/docs/Contributing.rst
Outdated
@@ -175,7 +175,7 @@ Introduction | |||
------------ | |||
|
|||
Unlike most parts of the LLVM project, libc++ uses a pre-commit CI [#]_. This | |||
CI is hosted on `Buildkite <https://buildkite.com/llvm-project/libcxx-ci>`__ and | |||
CI used to be hosted on `Buildkite <https://buildkite.com/llvm-project/libcxx-ci>`__, but has migrated to the LLVM CI infrastrcuture, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still some CI running on the libc++ Buildkite, mainly platforms that are difficult to run through Github Actions as far as I'm aware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, reflow this text to respect the line length limit if possible.
libcxx/docs/Contributing.rst
Outdated
runner groups. The three runner group names are | ||
"llvm-premerge-libcxx-runners", "llvm-premerge-libcxx-release-runners" | ||
or "llvm-premerge-libcxx-next-runners". Which runner set to use is | ||
controlled by the contents of https://github.com/llvm/llvm-project/blob/main/.github/workflows/libcxx-build-and-test.yaml . By default, it uses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reflow this text.
libcxx/docs/Contributing.rst
Outdated
To update just the runner binary, change the value of | ||
``ACTIONS_BASE_IMAGE`` to be one of the libcxx runner variable images | ||
from | ||
https://github.com/llvm/llvm-zorg/blob/main/premerge/premerge_resources/variables.tf. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a bit more specific than this, you'll want the builder-base
image at the same commit SHA.
So if you have ghcr.io/llvm/libcxx-linux-builder:16f046281bf1a11d344eac1bc44d11f3e50e3b5d
currently, you would want to set ACTIONS_BASE_IMAGE
to ghcr.io/llvm/libcxx-linux-builder-base:16f046281bf1a11d344eac1bc44d11f3e50e3b5d
libcxx/docs/Contributing.rst
Outdated
Lastly you need to create a PR in the llvm-zorg repository, | ||
updating the the value of the appropriate libcxx runner variable in | ||
the variables.tf file mentioned above. Once that change has been | ||
merged, an llvm-zorg administrator must use terraform to apply the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change llvm-zorg administrator to something like Googler/LLVM Premerge maintainer?
In my mind llvm-zorg administrator means Galina, and she doesn't have access to make changes to the premerge infra.
Add information to the libcxx testing documentation, about the names of the new CI libcxx runner sets, their current values, and how to change the values or the runner set being used.