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

add publish-crates workflow #595

Merged
merged 4 commits into from
Nov 1, 2023
Merged

Conversation

jbr
Copy link
Contributor

@jbr jbr commented Oct 24, 2023

This pr locks versions between the cli, the client, and the api, but that may not be desirable. I'd appreciate review on whether we should have distinct semver-meaningful versions for each crate or whether we want to bump everything all at once like janus. Also, we may want to have a discussion about what merits a semver-minor change, since we recently quietly introduced a breaking change to the api

@jbr jbr requested a review from a team as a code owner October 24, 2023 19:14
@jbr jbr force-pushed the add-release-script-for-publishable-crates branch 2 times, most recently from 05baa10 to 1f6e99c Compare October 24, 2023 19:33
Copy link
Contributor

@branlwyd branlwyd left a comment

Choose a reason for hiding this comment

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

Thoughts on versioning: I'm fine keeping the two crates in-sync in terms of version; as long as we're using 0.0.x versions we don't have any backwards-compatibility concerns (that is, every version is expected to be compatible only with itself).

We will need to provide better backcompat guarantees eventually, and we should do so by switching to (at least) a 0.x.y version. For now, I think we can stay with a 0.0.x version.

@@ -36,3 +36,4 @@ trillium-macros = "0.0.4"
trillium-testing = { version = "0.5.0", features = ["tokio"] }
trillium-rustls = "0.4.0"
trillium-tokio = "0.3.2"
divviup-client = { path = ".", features = ["admin"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

super-nit: missing trailing newline

@jbr
Copy link
Contributor Author

jbr commented Oct 24, 2023

My concern with 0.0.x is specifically for divviup-client, since cargo will auto update to the latest patch version, which will include breaking change versions as well as nonbreaking changes. It might make more sense to use 0.x.0 if we're only going to do one version number?

@branlwyd
Copy link
Contributor

👍🏻 Going to 0.1.0 sounds good to me.

@tgeoghegan
Copy link
Contributor

Yes, let's go to 0.1.0. Also, I like keeping the various crate versions in sync, because then it's very easy to tell people to use client or CLI version 0.x.y if they want to talk to a server running version 0.x.y and get a guarantee they speak the same version (in case we do mess up with semver).

@tgeoghegan
Copy link
Contributor

re: SemVer: Over in Janus, we've been trying to respect SemVer for Rust API changes, but also when we change DAP versions, since that breaks compatibility with other DAP participants. So I think we could also bump the minor (middle) version here when we know we've broken the HTTP API.

@jbr
Copy link
Contributor Author

jbr commented Oct 25, 2023

we also have our content type version, maybe we should use the semver-minor as the content type version, and bump both of those on breaking api change?

@tgeoghegan
Copy link
Contributor

That seems fine to me, though we'd also have to look into actually doing something with the content types.

@jbr
Copy link
Contributor Author

jbr commented Oct 25, 2023

That seems fine to me, though we'd also have to look into actually doing something with the content types.

We do enforce that the content types are identical, which provides a compatibility check at the api boundary. At some later point, individual endpoints could annotate which version ranges they support and/or switch implementations based on content type / eaccept. Erring in the direction of a stricter api compatibility contract seems preferable early-on while it's feasible to keep clients upgrading. Later, if it's difficult to keep clients upgrading at the pace of breaking api changes, there are options.

Copy link
Contributor

@tgeoghegan tgeoghegan left a comment

Choose a reason for hiding this comment

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

Should we take this before releasing 0.0.31 and deploying it this week? I'll need to rebase #611 but that's no big deal.

@jbr jbr force-pushed the add-release-script-for-publishable-crates branch from 1f6e99c to 2d710b7 Compare November 1, 2023 20:52
@jbr jbr mentioned this pull request Nov 1, 2023
@jbr jbr force-pushed the add-release-script-for-publishable-crates branch 2 times, most recently from a702e0d to b90042c Compare November 1, 2023 21:12
Cargo.toml Outdated
version = "0.0.30"
edition = "2021"
license = "MPL-2.0"
homepage = "https://divviup.org"
repository = "https://github.com/divviup/divviup-api"

[workspace.dependencies]
divviup-client = { path = "./client" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll still need to specify a version on these dependencies, in addition to specifying a path, in order to publish certain crates. Currently, cargo publish --dry-run -p divviup-cli fails because its divviup-client dependency doesn't specify a version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

drat, I was hoping more of https://rust-lang.github.io/rfcs/2906-cargo-workspace-deduplicate.html had been implemented than was

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we should do a publish dry run in ci?

Copy link
Collaborator

Choose a reason for hiding this comment

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

One barrier we've run into with Janus is that, since we're publishing two crates consecutively from the same workflow, there are some changes where the dry run for the second crate can't pass if the new version of the first crate hasn't been released.

@jbr jbr force-pushed the add-release-script-for-publishable-crates branch from b90042c to 9624798 Compare November 1, 2023 21:33
@divergentdave
Copy link
Collaborator

divergentdave commented Nov 1, 2023

I got cargo publish --dry-run -p divviup-client to work with the following changes.

diff --git a/Cargo.toml b/Cargo.toml
index 6ae9c44..b1782a3 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -11,8 +11,8 @@ repository = "https://github.com/divviup/divviup-api"
 [workspace.dependencies]
 divviup-client = { path = "./client", version = "0.0.30" }
 divviup-cli = { path = "./cli", version = "0.0.30" }
-divviup-api = { path = ".", version = "0.0.30" }
-test-support = { path = "./test-support", version = "0.0.30" }
+divviup-api.path = "."
+test-support.path = "./test-support"
 
 [package]
 name = "divviup-api"
diff --git a/client/Cargo.toml b/client/Cargo.toml
index 8f8c250..3eca422 100644
--- a/client/Cargo.toml
+++ b/client/Cargo.toml
@@ -36,4 +36,4 @@ trillium-macros = "0.0.4"
 trillium-testing = { version = "0.5.0", features = ["tokio"] }
 trillium-rustls = "0.4.0"
 trillium-tokio = "0.3.2"
-divviup-client =  { workspace = true, features = ["admin"] }
+divviup-client =  { path = ".", features = ["admin"] }

Including a version in addition to a path on private crates was a problem, because Cargo was still trying to look them up in the index. Only when a dev-dependency has just a path will Cargo prune it before publishing. Then, the self-dev-dependency that divviup-client had needed to drop the version for a similar reason, since the version it specified necessarily hadn't been uploaded yet.

@jbr jbr merged commit 036a350 into main Nov 1, 2023
6 checks passed
@jbr jbr deleted the add-release-script-for-publishable-crates branch November 1, 2023 21:50
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.

4 participants