This is an archive of the discontinued LLVM Phabricator instance.

github: Add manual workflow to build and upload release binaries
ClosedPublic

Authored by tstellar on Feb 7 2023, 4:09 PM.

Diff Detail

Event Timeline

tstellar created this revision.Feb 7 2023, 4:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2023, 4:09 PM
tstellar requested review of this revision.Feb 7 2023, 4:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2023, 4:09 PM
tstellar updated this revision to Diff 495677.Feb 7 2023, 4:19 PM

Rebase on main.

tstellar added a subscriber: str4d.EditedFeb 7 2023, 4:41 PM

This patch is from @str4d

.github/workflows/release-binaries.yml
4

Do we need to add:

push:
  tags:
      - 'llvmorg-*'
10

Should we add an option to specify the ref/tag ?

Thanks for working on this! I have a bunch of comments :)

.github/workflows/release-binaries.yml
10

+1

35–44

This block is very messy. I would l prefer if this ended up in a script or something instead. Going to be complicated to troubleshoot this logic when it lives in the action definition.

54

Is this the correct triple? I would have expected x86_64-unknown-linux-gnu or something similar instead. At least since we pass this directly as LLVM_DEFAULT_TARGET I think we need to be careful to get this right.

Probably better to have a buildname and triple separated.

88

if we pipe this to true won't we miss errors that happens here then?

tschuett added inline comments.
.github/workflows/release-binaries.yml
15

I would recommend to pin the image to, e.g. ubuntu-22.04.

tstellar updated this revision to Diff 497499.Feb 14 2023, 5:21 PM

Addresss some review feedback and also make sure the correct ref is checked out.

tstellar marked 2 inline comments as done.Feb 14 2023, 5:27 PM
tstellar added inline comments.
.github/workflows/release-binaries.yml
35–44

OK, I will move this to a script.

54

The triple is only used for naming the tarball, not for anything else.

88

Since this is being run after the tag has been created, I think it's OK to ignore failures because it's too late to make changes at this point. Also if we fail here, then the binaries won't be uploaded.

The alternative would be to drop the || true and change the 'Upload binaries' step to always run.

tstellar updated this revision to Diff 497537.Feb 14 2023, 9:42 PM

Put setup code into a script and make sure to always do the upload.

tstellar marked 2 inline comments as done.Feb 14 2023, 9:43 PM
tstellar updated this revision to Diff 498242.Feb 16 2023, 7:47 PM

Update permissions for the action.

I've add some comments about better scoped permissions.

.github/workflows/release-binaries.yml
18

Top level permissions should always be read only.

62

Write permissions should be given on run level (jobs).

tstellar updated this revision to Diff 499297.Feb 21 2023, 2:27 PM
tstellar marked 2 inline comments as done.

Fix permissions.

kwk added a subscriber: kwk.Feb 22 2023, 10:17 AM

Added some ideas. Maybe not all so good.

.github/workflows/release-binaries.yml
12
15
15
46
53

I wonder if this will work. When I today tried this I got false and true values. That's why I check differently. The string comparison is safe this way.

.github/workflows/set-release-binary-outputs.sh
25

This one is not listed in the outputs: section of of the prepare job.

tstellar updated this revision to Diff 501329.Feb 28 2023, 4:22 PM
tstellar marked 4 inline comments as done.

Address some more review comments.

tstellar added inline comments.Mar 9 2023, 10:02 PM
.github/workflows/release-binaries.yml
53

The intended logic is for upload to be true if inputs.upload is unset. Otherwise upload should equal inputs.upload.

.github/workflows/set-release-binary-outputs.sh
25

It's there now.

kwk requested changes to this revision.Mar 22 2023, 8:11 AM

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

.github/workflows/release-binaries.yml
11

Why is it optional to upload the binaries?

49–54

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.

101–108

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.

112–117

Same reason as above.

.github/workflows/set-release-binary-outputs.sh
16

This way the logic is less implicit or bashy and more easy on the eyes IMHO.

17

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?

This revision now requires changes to proceed.Mar 22 2023, 8:11 AM
tstellar updated this revision to Diff 509216.Mar 28 2023, 10:54 PM
tstellar marked 6 inline comments as done.

Address some review comments. @kwk I had to make some slight modifications to some
of your suggests, so let me know how this looks.

tstellar updated this revision to Diff 509228.Mar 29 2023, 12:14 AM

Address more review comments.

tstellar added inline comments.Mar 29 2023, 9:54 AM
.github/workflows/release-binaries.yml
11

I thought it would be useful for doing test builds that we don't want to end up on the official release page.

46

bash -e is the default shell command already.

.github/workflows/set-release-binary-outputs.sh
16

I had to change the logic slightly, but I implemented this change.

kwk accepted this revision.Apr 17 2023, 9:06 AM

Thank you Tom for accepting and improving on my feedback.

.github/workflows/release-binaries.yml
11

I thought it would be useful for doing test builds that we don't want to end up on the official release page.

Yes, that makes sense.

.github/workflows/set-release-binary-outputs.sh
16

I had to change the logic slightly, but I implemented this change.

Thank you. Much better.

This revision is now accepted and ready to land.Apr 17 2023, 9:06 AM
This revision was landed with ongoing or failed builds.Apr 17 2023, 12:04 PM
This revision was automatically updated to reflect the committed changes.