Skip to content
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

github: workflows: release: Add musl build for aarch64 #724

Closed
wants to merge 5 commits into from

Conversation

patrickelectric
Copy link

@patrickelectric patrickelectric commented Oct 19, 2023

Based on #713, github does not allow me reopen it (the branch name is the same 👀 )

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
… in one

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Copy link
Owner

@fujiapple852 fujiapple852 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrickelectric I've left a few comment but essentially I struggling to understand the intended behaviour of the changes here, shouldn't this be a simple case of adding the new targets?

If easier please feel free to ping me on discord or matrix to discuss (i'm fujiapple852 on both)

Comment on lines -4 to +5
tags:
- "[0-9]+.[0-9]+.[0-9]+"
pull_request:

Copy link
Owner

@fujiapple852 fujiapple852 Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrickelectric we only want to run the release workflow on a tag rather than on every pr. I assume you've done this to help you test? If so you'll need to do so on a fork as if we let this pr run now it would trigger an actual release which we do not want (it annoys package maintainers who are notified and start working on new package versions).

jobs:
create-release:
name: create-release
runs-on: ubuntu-latest
if: startsWith(github.ref, 'refs/tags/')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to undo the effect of the change above and only run the step for tags? I'm a bit confused as to what effect you're try to achieve here.

@@ -38,18 +39,28 @@ jobs:
TARGET_DIR: ./target
RUST_BACKTRACE: 1
strategy:
fail-fast: false
Copy link
Owner

@fujiapple852 fujiapple852 Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want fail-fast as true here (the default I suspect?). if a release partially fails I would want to fix it and rerun it rather than allowing a subset of packages to be published.

Comment on lines +120 to +125
- name: Upload Artifact
uses: actions/upload-artifact@v3
with:
name: trippy-${{ matrix.target }}
path: trippy-*
if-no-files-found: error
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? There is already an step to upload a release archive

os: ubuntu-22.04
target: aarch64-unknown-linux-gnu
- build: linux-arm64
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a duplicate build name, did you want linux-aarch64-musl perhaps?

- build: linux-armv7
os: ubuntu-22.04
target: armv7-unknown-linux-gnueabihf
- build: linux-armv7
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another duplicate name.

@fujiapple852
Copy link
Owner

Superseded by #810

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants