Skip to content

Conversation

@vHideyukiHoukawa
Copy link

This tool is awesome — thank you!

I’ve prepared a multi-platform release workflow with specific binary formats so that this tool can be registered in the aqua registry.

Here’s an example release published by this workflow:
https://github.com/vHideyukiHoukawa/tomli/releases/tag/v0.4.1-test-32

Build for Linux, macOS, Windows (AMD64/ARM64) with testing and checksums.
Copy link
Owner

@blinxen blinxen left a comment

Choose a reason for hiding this comment

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

Thanks very much for this PR! Glad you like the tool!

I left a couple of remarks / thoughts on some of the changes.

arch_name: amd64
file_ext: tar.gz
# macOS ARM64
- os: macos-14
Copy link
Owner

Choose a reason for hiding this comment

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

Any special reason this is macos-14 and not macos-latest?

strategy:
matrix:
include:
# Linux AMD64 (musl)
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason why Linux is built using musl?

Comment on lines +77 to +94
- name: Install Rust target and cross-compilation tools (Unix)
run: |
rustup target add ${{ matrix.rust_target }}
if [ "${{ matrix.rust_target }}" = "x86_64-unknown-linux-musl" ]; then
sudo apt-get update
sudo apt-get install -y musl-tools
elif [ "${{ matrix.rust_target }}" = "aarch64-unknown-linux-musl" ]; then
sudo apt-get update
sudo apt-get install -y musl-tools gcc-aarch64-linux-gnu
mkdir -p .cargo
echo '[target.aarch64-unknown-linux-musl]' > .cargo/config.toml
echo 'linker = "aarch64-linux-gnu-gcc"' >> .cargo/config.toml
fi
if: ${{ matrix.os != 'windows-latest' }}

- name: Install Rust target (Windows)
run: rustup target add ${{ matrix.rust_target }}
if: ${{ matrix.os == 'windows-latest' }}
Copy link
Owner

Choose a reason for hiding this comment

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

These two can be merged together

Comment on lines +110 to +116
elif [[ "${{ matrix.os }}" == macos-* ]]; then
cp target/${{ matrix.rust_target }}/release/${{ env.PROJECT_NAME }} ${BINARY_EXECUTABLE_NAME}
tar -czf ${ARCHIVE_NAME}.tar.gz ${BINARY_EXECUTABLE_NAME} CHANGELOG.md LICENSE
else # Linux
cp target/${{ matrix.rust_target }}/release/${{ env.PROJECT_NAME }} ${BINARY_EXECUTABLE_NAME}
tar -czf ${ARCHIVE_NAME}.tar.gz ${BINARY_EXECUTABLE_NAME} CHANGELOG.md LICENSE
fi
Copy link
Owner

Choose a reason for hiding this comment

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

These two look pretty much the same. You can probably merge them.

uses: actions/upload-artifact@v4
with:
name: ${{ env.PROJECT_NAME }}-${{ matrix.os }}-${{ matrix.arch_name }}
path: ${{ env.ARCHIVE_NAME }}.*
Copy link
Owner

Choose a reason for hiding this comment

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

What does the .* do here? I prefer to not use wildcards whenever possible.

name: ${{ env.PROJECT_NAME }}-darwin-universal
path: ${{ env.UNIVERSAL_ARCHIVE_NAME }}.tar.gz

test-binaries:
Copy link
Owner

Choose a reason for hiding this comment

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

I don't quite understand this step. It does the same as the integration tests in tests/. IMHO, this can be replaced by running the integration tests in the different environments.

tar -czf tomli.tar.gz tomli CHANGELOG.md LICENSE
echo "# SHA256 Checksums for ${{ env.PROJECT_NAME }} ${{ env.VERSION }}" > SHA256SUMS
echo "# Verify with: sha256sum -c SHA256SUMS" >> SHA256SUMS
echo "" >> SHA256SUMS
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
echo "" >> SHA256SUMS
echo >> SHA256SUMS

@blinxen
Copy link
Owner

blinxen commented Nov 9, 2025

Looking at the published archives, I would also prefer them following the convention NAME-VERSION-${TARGET_TRIPLE}.tar.gz.

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