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

chore: bump jf-vid version, update changelog #680

Merged
merged 7 commits into from
Sep 12, 2024
Merged

Conversation

ggutoski
Copy link
Contributor

@ggutoski ggutoski commented Sep 10, 2024

@@ -1,6 +1,6 @@
[package]
name = "jf-vid"
version = "0.1.0"
version = "0.2.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Version numbers vs git tags

[Copied from internal discussion.]

Currently each package in the jellyfish workspace (eg jf-vid, jf-pcs, etc) specifies its own version number (eg 0.1.0, etc). Moreover, some jellyfish packages depend on other jellyfish packages. Such a dependency is currently specified in Cargo.toml like so:

jf-pcs = { git = "https://github.com/EspressoSystems/jellyfish", tag = "0.4.5" }

External workspaces (eg espresso-sequencer) are similar, except they might also specify a version number:

jf-pcs = { version = "0.1.0", git = "https://github.com/EspressoSystems/jellyfish", tag = "0.4.5" }

I think this is not best practice. According to The Cargo Book:

The version key does not affect which commit is used when Cargo retrieves the git dependency, but Cargo checks the version information in the dependency’s Cargo.toml file against the version key and raises an error if the check fails.

Thus, no user of a jellyfish package (either inside or outside the jellyfish workspace) ever actually uses the package's version number! (Except possibly as a sanity check against a git commit.)

Also, it's awkward to bump a version number of a package in the jellyfish workspace. Any jellyfish package (eg jf-vid) with a dependency on another jellyfish package (eg. jf-pcs) must specify a git tag for an older commit of jf-pcs. Thus, the jellyfish workspace is not internally consistent: packages are not guaranteed to build against a single, fixed jellyfish commit. Any user of jf-vid must necessarily have at least two jellyfish git tags in its dependency graph. 🤦‍♂️

Opinions? @alxiong @mrain

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these are expected drawbacks after the major refactor. Perhaps we should name the tags as sth like jf-pcs-0.2.0

Copy link
Contributor

@alxiong alxiong Sep 11, 2024

Choose a reason for hiding this comment

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

Thus, no user of a jellyfish package (either inside or outside the jellyfish workspace) ever actually uses the package's version number!

it's true that version number is ignored for now, but when we eventually publish these crates, the version number will matter. Before publishing, we sync the tag name with the version number as git = "", tag = "" pins to a specific version.

Thus, the jellyfish workspace is not internally consistent: packages are not guaranteed to build against a single, fixed jellyfish commit.

not true? we always created tag from some commit on main (that commit is the single commit that is guaranteed to be built successfully).
e.g. https://github.com/EspressoSystems/jellyfish/releases/tag/jf-plonk-v0.5.1, after adding this tag, we didn't force downstream to update all jf-* to point to this tag (if they do, it would still compile), but only jf-plonk to point to it. because the jf-plonk at this commit is pointing to some earlier tag of other crates jf-*.

if we update jf-crhf and cause jf-plonk to break, we would bump versions for both.

Any user of jf-vid must necessarily have at least two jellyfish git tags in its dependency graph.

that's expected outcome when we decide to use separate version bumps for every package/crates. I don't see this as a problem -- it happens in your nix/store as well. Different tools depend on different versions of curl, thus you have multiple versions of curl under the same nix shell environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because the jf-plonk at this commit is pointing to some earlier tag of other crates jf-*.

That's my point. We have lost the following property: there exists commit x such that jf-plonk at commit x is guaranteed to compile with all its jf-* dependencies also at commit x.

Perhaps there's nothing fundamentally wrong about this beyond dependency graph bloat, but it feels icky to me---like a symptom of poorly-maintained code. I guess I should stop thinking of jellyfish as a single, cohesive library and instead view it as a bunch of completely independent packages that happen to share the same git repo.

I consider this question resolved. Thanks for your comments @alxiong !

Suggestion: add version key to all jf-* dependencies in the jellyfish repo. This suggestion is more consistent with idiomatic Rust, and will make it easier to switch from git commit tags to version numbers when jellyfish crates eventually get published to a registry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestion: add version key to all jf-* dependencies in the jellyfish repo. This suggestion is more consistent with idiomatic Rust, and will make it easier to switch from git commit tags to version numbers when jellyfish crates eventually get published to a registry.

done in dc7130c

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm a little mixed with "adding version key", it reads confusing. version = 0.4.4, tag = 0.4.5
this is the idiomatic way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's only in jf-utils package. Unlike all other packages, jf-utils inherits its version from the workspace:

version = { workspace = true }

...and the workspace at git tag 0.4.5 declares its version to be 0.4.4 🤦 :
version = "0.4.4"

I propose that jf-utils do not inherit its version from the workspace. Instead, we should reset its version to 0.1.0 like all the other packages in this repo.

Question

What should we do with the workspace-level version? It seems kinda useless now because each package explicitly sets its own version. Options:

  1. Set it to 0.4.6 in so that it matches a future new commit tag for this repo. But this version is meaningless because no package actually uses it.
  2. Set it to 0.1.0, since that's the version number for most packages in this workspace. If we do this, do we then revisit all packages currently set to 0.1.0 and change them to inherit from the workspace? Seems obfuscated for no real benefit.
  3. Delete version key entirely from the workspace.

Copy link
Contributor

Choose a reason for hiding this comment

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

I propose that jf-utils do not inherit its version from the workspace. Instead, we should reset its version to 0.1.0 like all the other packages in this repo.

i agree. cc @mrain

Delete version key entirely from the workspace.

I choose this one. since RustCrypto does so. https://github.com/RustCrypto/hashes/blob/master/Cargo.toml, separate version management for each member.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted version key from workspace in ba7ffb7.

On further consideration, I think we should keep jf-utils at 0.4.4. This package has existed since the beginning, so to reset its version number now would be bad. There is precedent: jf-plonk also kept its 0.4.4 version number after the restructuring of this workspace. (jf-plonk is now at 0.5.1 whereas jf-utils is still at 0.4.4.)

Unfortunately, the stupidity remains with version = 0.4.4, tag = 0.4.5 in downstream Cargo.toml files. 😕

Proposal

Now that package version numbers are completely decoupled from the workspace, we should stop naming jellyfish tags according to semver because it creates confusion. I don't know what tag name we should use instead. It's hard to pick a good name for tags because we need a new tag each time any of the packages bumps its version. One idea is to create a separate tag for each package, so that downstream Cargo.toml files look like

jf-rescue = { version = "0.1.0", tag = "jf-rescue-0.1.0", git = ...
jf-utils = { version = "0.4.4", tag = "jf-utils-0.4.4", git = ...

Initially, most of these tags will point to a single commit. For example, both jf-rescue-0.1.0 and jf-utils-0.4.4 will point to the commit currently tagged as 0.4.5. Yes, this means lots of redundant tags. But the result is much more readable and the usage pattern is intuitive.

Opinions? @alxiong @mrain

Copy link
Contributor

Choose a reason for hiding this comment

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

One idea is to create a separate tag for each package,

agree, that's exactly what we've been doing: https://github.com/EspressoSystems/jellyfish/tags (and what RustCrypto does: https://github.com/RustCrypto/hashes/tags)

vid/CHANGELOG.md Outdated Show resolved Hide resolved
@ggutoski ggutoski merged commit 97ee6a2 into main Sep 12, 2024
5 checks passed
@ggutoski ggutoski deleted the gg/jf-vid-0.2.0 branch September 12, 2024 21:08
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.

3 participants