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: add publishing workflow (triggered by tags) #248

Merged
merged 3 commits into from
Dec 18, 2023

Conversation

msiglreith
Copy link
Contributor

Add a github workflow for publishing the crates semi-automatically. Currently went for tag based trigger due to metadata changes to Cargo.toml but open to switch to the 'normal' Cargo.toml trigger.

Secret already added in the settings.

@MarijnS95
Copy link
Collaborator

MarijnS95 commented Dec 4, 2023

but open to switch to the 'normal' Cargo.toml trigger.

That might work now that changes in version metadata are correctly rejected by crates.io.... But we've had some problems with exactly such a trigger in the past on the ndk-sys crate :(

- name: Publish spirv
run: cargo publish --manifest-path spirv/Cargo.toml --token ${{ secrets.cratesio_token }}
- name: Publish rspirv
run: cargo publish --manifest-path rspirv/Cargo.toml --token ${{ secrets.cratesio_token }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a final newline maybe?

on:
push:
tags:
- '*'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just tags: should be enough. And maybe only when Cargo.toml changes?

Suggested change
- '*'

https://github.com/Traverse-Research/rust-template/blob/main/.github/workflows/publish.yaml

(That path needs to be changed for the workspace crates though)

@MarijnS95
Copy link
Collaborator

Note that we use cargo-release with a publish = false. It bumps the versions, replaces them in README.md, creates and signs a commit and tag, pushes the whole bunch to GitHub and then relies on a very similar publish workflow to push it to crates.io :)

@msiglreith
Copy link
Contributor Author

Thanks for suggestions, I added the Cargo.toml trigger.
I need to look at cargo-release at some point, haven't used it so far - also in combination with cargo workspaces

Comment on lines +4 to +6
push:
tags:
paths: "**/Cargo.toml"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do wonder if this now runs twice when a tag for spirv and rspirv are pushed concurrently. Maybe we can use GitHub's concurrency: feature with the commit SHA as hash when they are both pointing to the same tag? (And we should try to avoid two different dependent commits that bump each tag individually)

@msiglreith
Copy link
Contributor Author

Did some tests with cargo-release at https://github.com/msiglreith/ci-test to emulate how the release handling might work for rspirv, final command was cargo release minor --exclude "c" -x --no-publish

  • adding the SHA based concurrency protection correctly cancel the redundant CI calls (called 3 times due to branch and both tags)
  • bumping the version via cargo release minor doesn't preserve the version metadata (+sdk-..)
  • bumping the version also doesn't seem to update the versions specified at dependencies (ie rspirv depending on spirv)

@MarijnS95
Copy link
Collaborator

The repo I linked also has a release.toml to configure things like --no-publish. I've never used minor to bump though, always explicit versions and maybe some tweaking is needed to maintain version metadata. However, not sure how one would use that to bump two crates to different versions and push two different tags with the crate prefix.

For replacing the version of spirv inside the rspirv crate, maybe pre-release-replacements works but I hope there's something smarter for multiple crates (or maybe cargo-release doesn't fully support multiple crates at all).

@msiglreith
Copy link
Contributor Author

Just noticed that there already are existing release.toml files in spirv/rspirv after re-reading the MAINTENANCE.md🙃
Added the concurrency filter and disable publishing via cargo-release along with trying to improve the documentation a bit

@MarijnS95
Copy link
Collaborator

Ou yeah, that must have actually been the origin of these files in our repo...

But it does mean you cannot publish both crates with one command at once... I wonder if cargo-release actually supports this nowadays.

@msiglreith
Copy link
Contributor Author

But it does mean you cannot publish both crates with one command at once... I wonder if cargo-release actually supports this nowadays.

Unfortunately it doesn't seem to support this workflow properly currently, ideally we would be able publish both together or also individually, but this decoupled configuration seems currently not available. I would go forward with the existing workflow as I currently don't see a straightforward way to improve it.

@msiglreith msiglreith merged commit 06533f2 into gfx-rs:master Dec 18, 2023
9 checks passed
@msiglreith msiglreith deleted the ci-publish branch December 18, 2023 17:22
@MarijnS95
Copy link
Collaborator

There are mentions of workspaces in the docs but I'm not fully understanding if, and if so how, it could bump internal dependencies to different versions.

@MarijnS95
Copy link
Collaborator

Hmm since there is cargo release minor it might be possible, rather than having to pass an explicit version (e.g. cargo release 0.3.0) which is what I am used to.

@msiglreith
Copy link
Contributor Author

Right, that was one of the issues I faced as well when trying cargo release minor (#248 (comment)). The current release.toml configs would handle the internal dependency bump but seems to cause some other issues along with losing the +sdk suffix:

   Upgrading rspirv-autogen from 0.1.0 to 0.2.0
   Upgrading spirv from 0.2.0+sdk-1.3.268.0 to 0.3.0
    Updating rspirv's dependency from 0.2.0 to 0.3.0
   Upgrading rspirv from 0.11.0+sdk-1.3.268.0 to 0.12.0
   Upgrading rspirv-dis from 0.1.0 to 0.2.0
   Replacing in ../rspirv/Cargo.toml
--- ../rspirv/Cargo.toml        original
+++ ../rspirv/Cargo.toml        replaced
@@ -23 +23 @@
-spirv = { version = "0.2.0", path = "../spirv" }
+spirv = { version = "0.3.0", path = "../spirv" }

   Replacing in README.md
--- README.md   original
+++ README.md   replaced
@@ -21 +21 @@
-spirv = "0.2.0"
+spirv = "0.3.0"

   Replacing in README.md
--- README.md   original
+++ README.md   replaced
@@ -37 +37 @@
-rspirv = "0.11.0"
+rspirv = "0.12.0"

  Publishing rspirv-dis
    Updating crates.io index
error: all dependencies must have a version specified when publishing.
dependency `rspirv` does not specify a version
Note: The published dependency will use the version from crates.io,
the `path` specification will be removed from the dependency declaration.

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