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

ci: upload jaq executable to GitHub releases #102

Closed
wants to merge 15 commits into from

Conversation

eitsupi
Copy link

@eitsupi eitsupi commented Aug 5, 2023

Add a new workflow to upload jaq executable files to GitHub releases.

I have tested on my fork.

image

@eitsupi eitsupi marked this pull request as ready for review August 5, 2023 09:16
Comment on lines +48 to +55
- name: Build
if: runner.os != 'Linux'
run: cargo build --release --locked --target ${{ matrix.target }}
shell: bash
- name: Build with cross
if: runner.os == 'Linux'
run: cross build --release --locked --target ${{ matrix.target }}
shell: bash
Copy link
Owner

Choose a reason for hiding this comment

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

This looks redundant?

Copy link
Author

Choose a reason for hiding this comment

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

The commands are different if closely looked at because they use CROSS only on Linux.
It is certainly possible to combine them into one step using if, but I am concerned that it would reduce readability.

Comment on lines +56 to +72
- name: Create artifact for Linux and macOS
shell: bash
if: runner.os != 'Windows'
run: |
export ARTIFACT_NAME="jaq-${{ github.ref_type == 'tag' && github.ref_name || 0 }}-${{ matrix.target }}.tar.gz"
echo "ARTIFACT_NAME=${ARTIFACT_NAME}" >>"$GITHUB_ENV"
cd target/${{ matrix.target }}/release
tar czf "../../../${ARTIFACT_NAME}" jaq ../../../LICENSE-MIT -C ../../../ README.md

- name: Create artifact for Windows
shell: bash
if: runner.os == 'Windows'
run: |
export ARTIFACT_NAME="jaq-${{ github.ref_type == 'tag' && github.ref_name || 0 }}-${{ matrix.target }}.zip"
echo "ARTIFACT_NAME=${ARTIFACT_NAME}" >>"$GITHUB_ENV"
cd target/${{ matrix.target }}/release
7z a "../../../${ARTIFACT_NAME}" jaq.exe ../../../LICENSE-MIT ../../../README.md
Copy link
Owner

Choose a reason for hiding this comment

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

Can you merge these two steps? Perhaps using something like:

        with:
          version: ${{ github.ref_type == 'tag' && github.ref_name || 0 }}
          archive: ${{ runner.os == 'Windows' && 'zip' || 'tar.gz' }}
          # ... (similar for 7z vs. tar, .exe etc.)
        run: |
          export ARTIFACT_NAME="jaq-${{ version }}-${{ matrix.target }}.${{ archive }}"
          # ...

Copy link
Author

Choose a reason for hiding this comment

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

I am sorry but I do not know how to do it.

Comment on lines +62 to +63
cd target/${{ matrix.target }}/release
tar czf "../../../${ARTIFACT_NAME}" jaq ../../../LICENSE-MIT -C ../../../ README.md
Copy link
Owner

Choose a reason for hiding this comment

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

Omit the cd and adapt the paths accordingly?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I couldn't figure out how to do the same thing without using cd.
Feel free to edit this PR to modify it.

@01mf02
Copy link
Owner

01mf02 commented Aug 7, 2023

Do you know how I can test this workflow without creating a release?

Copy link
Author

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

Do you know how I can test this workflow without creating a release?

Unfortunately I don't think there is a way to do that.
I am publishing the results of running this workflow in my fork for now so you can check it out.
Or, you should be able to copy this branch to a new repository and run it yourself.

Comment on lines +48 to +55
- name: Build
if: runner.os != 'Linux'
run: cargo build --release --locked --target ${{ matrix.target }}
shell: bash
- name: Build with cross
if: runner.os == 'Linux'
run: cross build --release --locked --target ${{ matrix.target }}
shell: bash
Copy link
Author

Choose a reason for hiding this comment

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

The commands are different if closely looked at because they use CROSS only on Linux.
It is certainly possible to combine them into one step using if, but I am concerned that it would reduce readability.

Comment on lines +56 to +72
- name: Create artifact for Linux and macOS
shell: bash
if: runner.os != 'Windows'
run: |
export ARTIFACT_NAME="jaq-${{ github.ref_type == 'tag' && github.ref_name || 0 }}-${{ matrix.target }}.tar.gz"
echo "ARTIFACT_NAME=${ARTIFACT_NAME}" >>"$GITHUB_ENV"
cd target/${{ matrix.target }}/release
tar czf "../../../${ARTIFACT_NAME}" jaq ../../../LICENSE-MIT -C ../../../ README.md

- name: Create artifact for Windows
shell: bash
if: runner.os == 'Windows'
run: |
export ARTIFACT_NAME="jaq-${{ github.ref_type == 'tag' && github.ref_name || 0 }}-${{ matrix.target }}.zip"
echo "ARTIFACT_NAME=${ARTIFACT_NAME}" >>"$GITHUB_ENV"
cd target/${{ matrix.target }}/release
7z a "../../../${ARTIFACT_NAME}" jaq.exe ../../../LICENSE-MIT ../../../README.md
Copy link
Author

Choose a reason for hiding this comment

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

I am sorry but I do not know how to do it.

@01mf02
Copy link
Owner

01mf02 commented Aug 22, 2023

Hi @eitsupi, thanks again for this PR. Given that this is a quite security-sensitive change, I decided to fully reimplement this functionality in order to understand how to use this safely. This is done in #107. I will prefer my PR to yours and hope that you understand. Still thank you for the inspiration. :)

@01mf02 01mf02 closed this Aug 22, 2023
@eitsupi eitsupi deleted the release-bin branch August 22, 2023 11:56
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