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

Bump version numbers for release #216

Merged
merged 5 commits into from
Jul 7, 2023
Merged

Bump version numbers for release #216

merged 5 commits into from
Jul 7, 2023

Conversation

alexytsu
Copy link
Collaborator

@alexytsu alexytsu commented Jul 5, 2023

Preps new version numbers to land once we have switched to stable rust

TODO

  • land stable rust toolchain and build wasm actors with new method

@alexytsu alexytsu marked this pull request as ready for review July 5, 2023 04:37
@alexytsu
Copy link
Collaborator Author

alexytsu commented Jul 5, 2023

@Stebalien I'm a little confused as to the versioning of crates such as fvm_shared, fvm, fvm_sdk.

Within a given project, is it recommended/necessary to have only one minor version of any of these particular crates? I.e. if a project pulls in dependencies that specify both fvm_shared@3.2 and fvm_shared@3.3 is this ok or are they considered incompatible?

In that case, in this repo we generally bump major versions when upgrading these dependencies to signal to our consumers that these changes are incompatible and need to be updated in line with the fvm crates. However, would bumping minor versions in a PR here be sufficient?

@alexytsu alexytsu requested a review from Stebalien July 5, 2023 04:41
@coveralls
Copy link

coveralls commented Jul 5, 2023

Coverage Status

coverage: 87.201% (+0.07%) from 87.128% when pulling 9f8310e on alex/update-fvm-ipld into acd5920 on main.

@alexytsu alexytsu changed the title chore: update fvm/ipld crates Bump version numbers for release Jul 5, 2023
@Stebalien
Copy link
Member

In that case, in this repo we generally bump major versions when upgrading these dependencies to signal to our consumers that these changes are incompatible and need to be updated in line with the fvm crates. However, would bumping minor versions in a PR here be sufficient?

Yeah.... See filecoin-project/ref-fvm#1724.

Basically, we've been versioning the FVM 1, 2, 3, etc. with for major incompatible network upgrades. Which means API breaking changes get minor version bumps. And yeah, this is incorrect.

My goal is to turn all of this into fvm1* and fvm2* crates using major versions for API-breaking changes. But that work keeps getting delayed behind more pressing stuff.

Stebalien and others added 4 commits July 6, 2023 10:31
I'm also moving all the version specifications to the workspace to
ensure we always use the same versions and to make future updates
easier.
Comment on lines +30 to +33
[patch.crates-io]
fvm_actor_utils = { git = "https://github.com/helix-onchain/filecoin", branch = "alex/update-fvm-ipld" }
frc42_dispatch = { git = "https://github.com/helix-onchain/filecoin", branch = "alex/update-fvm-ipld" }
frc46_token = { git = "https://github.com/helix-onchain/filecoin", branch = "alex/update-fvm-ipld" }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we need builtin-actors filecoin-project/builtin-actors#1330 to be updated to the same versions of FVM in order to build integration tests here. (Though technically we should be able to release the packages off this branch without passing the integration tests).

That builtin-actors commit patches back to this branch in order to bump its use of helix-libraries alongside underlying FVM versions. The issue now is that helix-filecoin depends on bactors (as a git patch) which depends back on helix-filecoin (as a git patch). However, when building from helix-filecoin, the bactors -> helix-filecoin dep is trying to pull in the helix crates from crates.io. Hence here we have to patch back to ourselves.

After the helix crates are released to crates.io we should then

@alexytsu
Copy link
Collaborator Author

alexytsu commented Jul 6, 2023

@Stebalien could you ptal

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM. One thing we should consider would be fixing the builtin actor's build system to apply patches when building actors. Substrate's wasm-builder does that but we were going for a "clean build". Bug given that the build isn't reproducible anyways.... it likely doesn't matter.

@Stebalien
Copy link
Member

After the helix crates are released to crates.io we should then

Note: I'm not sure how cargo will react if you try to publish with the patch in-place. You may have to release with broken CI.

@alexytsu alexytsu merged commit c1b1183 into main Jul 7, 2023
4 checks passed
@alexytsu alexytsu deleted the alex/update-fvm-ipld branch July 7, 2023 06:50
@alexytsu alexytsu restored the alex/update-fvm-ipld branch July 7, 2023 06:51
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