[PATCH] D143535: github: Add manual workflow to build and upload release binaries

[Konrad Wilhelm Kleine via Phabricator via llvm-commits]

kwk requested changes to this revision.
kwk added a comment.
This revision now requires changes to proceed.

@tstellar Sorry for the late reply. But I have some thoughts.



================
Comment at: .github/workflows/release-binaries.yml:10
+      upload:
+        description: 'Upload binaries to the release page'
+        required: true
----------------
Why is it optional to upload the binaries?


================
Comment at: .github/workflows/release-binaries.yml:48-53
+      run: |
+        if [ -n "${{ inputs.tag }}" ]; then
+          tag="${{ inputs.tag }}"
+        else
+          tag="${{ github.ref_name }}"
+        fi
----------------
I like the shorter syntax and the fact that we check for trimmed whitespaces. But if Github removes them automatically that's fine. These kind of problems only arise when you have the workflow inputs. Everytime I wrote one I've removed it in the end because of an insane overhead of sanity checking in awkward places like workflow files. 


================
Comment at: .github/workflows/release-binaries.yml:100-107
+      run: >
+        ${{ needs.prepare.outputs.build-dir }}/llvm-project/llvm/utils/release/test-release.sh
+        -release ${{ needs.prepare.outputs.release }}
+        ${{ needs.prepare.outputs.rc-flags }}
+        -triple ${{ matrix.target.triple }}
+        -use-ninja
+        -no-checkout
----------------
This way it is closer to what you can paste in a console. Also, the folded style with `>` is very odd IMHO. See this for example:

> To get a newline using the folded style, leave a blank line by putting two newlines in. Lines with extra indentation are also not folded.


================
Comment at: .github/workflows/release-binaries.yml:111-116
+      run: >
+        ${{ needs.prepare.outputs.build-dir }}/llvm-project/llvm/utils/release/github-upload-release.py
+        --token ${{ github.token }}
+        --release ${{ needs.prepare.outputs.release-version }}
+        upload
+        --files ${{ needs.prepare.outputs.build-dir }}/clang+llvm-${{ needs.prepare.outputs.release-version }}-${{ matrix.target.triple }}.tar.xz
----------------
Same reason as above. 


================
Comment at: .github/workflows/set-release-binary-outputs.sh:15
+
+test "$github_user" = "tstellar" || test "$github_user" = "tru"
+echo "$tag" | grep -e '^llvmorg-[0-9]\+\.[0-9]\+\.[0-9]\+\(-rc[0-9]\+\)\?$'
----------------
This way the logic is less implicit or bashy and more easy on the eyes IMHO.


================
Comment at: .github/workflows/set-release-binary-outputs.sh:16
+test "$github_user" = "tstellar" || test "$github_user" = "tru"
+echo "$tag" | grep -e '^llvmorg-[0-9]\+\.[0-9]\+\.[0-9]\+\(-rc[0-9]\+\)\?$'
+release_version=`echo "$tag" | sed 's/llvmorg-//g'`
----------------
What's with the echo here? It doesn't do anything, except that grep will exit with sonething different than 0 and that causes the script to exit, right? That's bad style IMHO. The fact that bash exits on failing commands shouldn't be exploited here. What if this line got removed one day because someone thinks it is not needed? Or what if the `-e` gets remove, then we simply run over this line. Let's make it explicit and more verbose?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143535