For each release tag, this action will create a new release on GitHub,
and for each -final tag, this action will build the documentation and
upload it to GitHub.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I'm not familiar with github actions so I'm just skimming the code, but it looks good as far as I can tell.
Any reason not to upload the docs also for release candidates? I think it's useful for people to be able to see the current state of release notes (usually to suggest they need improvement).
No reason not to. I was just trying to match the old behavior, but I can easily make that change.
.github/workflows/release-tasks.yml | ||
---|---|---|
18 | Do we have a way to ensure sane tagging? If not, then a tag like llvmorg-13.0.....1 would match the pattern as well. Do we want to make the versioning scheme more descriptive here? For example, do we always want MAJOR.MINOR.PATCH and optional -RC<0-9>? Then I suggest to change the grep expression to grep -e '^refs/tags/llvmorg-[0-9]\+\.[0-9]\+\.[0-9]\(-rc[0-9]\+\)\?$'. With explicit begin ^ and end $ this will actually result in non-matching tags to be discarded. | |
24 | Shouldn't this be sudo apt-get install -y to run in non-interactive mode? | |
30 | This line can be omitted because 1 is the default: https://github.com/actions/checkout#usage. | |
34 | Remove ./ | |
38 | I don't see this script in https://github.com/llvm/llvm-project/tree/main/llvm/utils/release nor in this change. What am I missing? |
.github/workflows/release-tasks.yml | ||
---|---|---|
38 | This script has been committed to the tree. |
I don't see serious problems with this change. And my comments are just improvements. Good Work!
.github/workflows/release-tasks.yml | ||
---|---|---|
25 | This is just a side question: Don't you have to apt-get update when in a fresh Ubuntu container? At least that is the case when starting it with podman or docker. Is that already done when you're running this with Github? When I do this locally I get: Reading package lists... Done Building dependency tree... Done Reading state information... Done E: Unable to locate package doxygen E: Unable to locate package graphviz E: Unable to locate package python3-github E: Unable to locate package python3-recommonmark E: Unable to locate package python3-sphinx E: Unable to locate package ninja-build E: Unable to locate package texlive-font-utils | |
30 | I've looked at the build-docs.sh script and it lists the sphinx-common package for Ubuntu installations as a requirement: apt-get install doxygen sphinx-common python3-recommonmark \ ninja-build graphviz texlive-font-utils pip3 install --user sphinx-markdown-tables Is this installed implicitly by any of the other packages that you install here or is it resolved when running the pip3 command? UPDATE: When I check with the packages that you've installed I can confirm that the dependency *is* installed: # dpkg -s sphinx-common Package: sphinx-common Status: install ok installed Priority: optional Section: python Installed-Size: 3110 Maintainer: Debian Python Team <team+python@tracker.debian.org> Architecture: all Multi-Arch: foreign Source: sphinx Version: 4.3.2-1 Replaces: python-sphinx (<< 1.1) Provides: dh-sequence-sphinxdoc Depends: libjs-sphinxdoc (= 4.3.2-1), perl:any Recommends: python3-sphinx Conflicts: python-sphinx (<< 1.1) Description: documentation generator for Python projects - common data Sphinx is a tool for producing documentation for Python projects, using reStructuredText as markup language. . This package includes manual pages, templates, translations and other data files. Homepage: https://www.sphinx-doc.org/ I have looked at this locally in a podman container and made a strange observation: Setting up x11-common (1:7.7+23ubuntu2) ... debconf: unable to initialize frontend: Dialog debconf: (No usable dialog-like program is installed, so the dialog based frontend cannot be used. at /usr/share/perl5/Debconf/FrontEnd/Dialog.pm line 78.) debconf: falling back to frontend: Readline [... OTHER PACKAGES ...] Setting up tzdata (2022a-0ubuntu1) ... debconf: unable to initialize frontend: Dialog debconf: (No usable dialog-like program is installed, so the dialog based frontend cannot be used. at /usr/share/perl5/Debconf/FrontEnd/Dialog.pm line 78.) debconf: falling back to frontend: Readline Configuring tzdata ------------------ Please select the geographic area in which you live. Subsequent configuration questions will narrow this down by presenting a list of cities, representing the time zones in which they are located. 1. Africa 2. America 3. Antarctica 4. Australia 5. Arctic Ocean 6. Asia 7. Atlantic Ocean 8. Europe 9. Indian Ocean 10. Pacific Ocean 11. US 12. None of the above Geographic area: The CLI asked for feedback even though I gave apt the assume-yes-switch. Let's just hope this doesn't happen in Github with the Ubuntu containers. Sorry for the noise, I'm just documenting this to confirm a thorough review. | |
44 | Why leave it up to the caller to decide about the interpreter? Shouldn't the script tell what to use with its shebang line? Right now the shebang line in build-docs.sh is #!/bin/sh. I suggest to change this to #!/bin/bash and get rid of the bash here. |
.github/workflows/release-tasks.yml | ||
---|---|---|
25 | I'm not sure that the runners are using containers, they may be using virtual machines. Either way, there have been a lot of packages pre-installed, so apt-get update must have been run at some point. | |
30 | You have to set DEBIAN_FRONTEND=noninteractive to skip these kind of questions (even with -y). The github runners do this: https://github.com/actions/virtual-environments/pull/366 |
Do we have a way to ensure sane tagging? If not, then a tag like llvmorg-13.0.....1 would match the pattern as well. Do we want to make the versioning scheme more descriptive here? For example, do we always want MAJOR.MINOR.PATCH and optional -RC<0-9>? Then I suggest to change the grep expression to grep -e '^refs/tags/llvmorg-[0-9]\+\.[0-9]\+\.[0-9]\(-rc[0-9]\+\)\?$'. With explicit begin ^ and end $ this will actually result in non-matching tags to be discarded.