Skip to content

Fix CI for master for the cargo publish job #997

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

simlay
Copy link
Member

@simlay simlay commented Aug 3, 2025

Related to #994.

It's not a full fix for #994 as I think git tags on versions would be ideal but this is a good quickfix for having red builds.

This is pretty much based off the publish job from coreaudio-rs.

I also added dependabot. @roderickvd this is likely to trigger a bunch of PRs. I'm generally a fan of this as it informs me what dependencies require actual work and which that don't.

@roderickvd
Copy link
Collaborator

Makes sense to do a "hotfix" now and revamp the rest of the workflow later.

From a plain code review, and reading the action documentation at https://github.com/katyo/publish-crates, I am confused. Looking at the documentation it should already fail gracefully when there's no new version to publish, even without continue-on-error. What am I missing?

@simlay
Copy link
Member Author

simlay commented Aug 4, 2025

Honestly, I'm a fan of run: cargo publish --token $CRATESIO_TOKEN with the continue-on-error like coreaudio-rs does when trying to publish again. Looking at #694, the script before checked if it had been published so I see the rational behind reducing some cruft.

@roderickvd
Copy link
Collaborator

You mean you'd prefer to have that manual cargo publish command rather than the magic of an action?

@simlay
Copy link
Member Author

simlay commented Aug 5, 2025

You mean you'd prefer to have that manual cargo publish command rather than the magic of an action?

Yup. The action seems pretty extensive and I bet is quite nice for a cargo workspace but there's only one crate in cpal.

@roderickvd
Copy link
Collaborator

I agree with that. Call me old-fashioned, but for something as impactful as pushing a release, I don't mind to take full control.

@roderickvd
Copy link
Collaborator

Just to be on the same page, you wanna change that in this PR, or would prefer this PR to be accepted as "quick fix" and look into more controlled release management later?

@simlay
Copy link
Member Author

simlay commented Aug 7, 2025

Yeah. If you've got publish rights on crates-io let's just pull the stuff publish ci out for a quick fix

@roderickvd
Copy link
Collaborator

Haven't got those rights yet, but we can still use the GitHub secret in the action.

@roderickvd
Copy link
Collaborator

That latest commit would trigger cargo publish on every commit to master, right?

If we were to adopt GitHub Releases (and tags) then we could use release triggers. Something like:

  cargo-publish:
    if: github.event_name == 'release'
    needs: [clippy-test, rustfmt-check, ubuntu-test]  # Wait for tests to pass
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v4
    - name: Install rust
      uses: dtolnay/rust-toolchain@stable
    - name: Update apt
      run: sudo apt update
    - name: Install alsa
      run: sudo apt-get install libasound2-dev
    
    # Verify the release tag matches Cargo.toml version
    - name: Verify release version
      run: |
        CARGO_VERSION=$(cargo metadata --no-deps --format-version 1 | jq -r '.packages[0].version')
        RELEASE_VERSION=${GITHUB_REF#refs/tags/v}
        echo "Cargo.toml version: $CARGO_VERSION"
        echo "Release tag version: $RELEASE_VERSION"
        if [ "$CARGO_VERSION" != "$RELEASE_VERSION" ]; then
          echo "Version mismatch! Cargo.toml has $CARGO_VERSION but release tag is v$RELEASE_VERSION"
          exit 1
        fi
    
    - name: cargo publish
      run: cargo publish --token ${{ secrets.CRATESIO_TOKEN }}

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