Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
.github/workflows/release-binaries.yml | ||
---|---|---|
15 | I would recommend to pin the image to, e.g. ubuntu-22.04. |
.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. |
Added some ideas. Maybe not all so good.
.github/workflows/release-binaries.yml | ||
---|---|---|
13 | ||
16 | ||
16 | ||
47 | ||
54 | 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 | ||
26 | This one is not listed in the outputs: section of of the prepare job. |
@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:
| |
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? |
Address some review comments. @kwk I had to make some slight modifications to some
of your suggests, so let me know how this looks.
.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. | |
47 | 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. |
Thank you Tom for accepting and improving on my feedback.
.github/workflows/release-binaries.yml | ||
---|---|---|
11 |
Yes, that makes sense. | |
.github/workflows/set-release-binary-outputs.sh | ||
16 |
Thank you. Much better. |
Do we need to add: