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

Umbrella crate issues #6987

Open
seunlanlege opened this issue Dec 23, 2024 · 29 comments
Open

Umbrella crate issues #6987

seunlanlege opened this issue Dec 23, 2024 · 29 comments

Comments

@seunlanlege
Copy link
Contributor

There are quite a few problems with the umbrella crate although it is a good step in the right direction. There are a few issues

1. Umbrella crate doesn't work with macros

I recently updated one of our libraries (polytope-labs/sc-simnode#48) to the umbrella crate only to discover it is incompatible with the macros in:

  • sp-runtime-interface
  • cumulus-pallet-parachain-system

2. Missing features for individual crates

The umbrella crate does not allow for using crate-specific features, for example sp-core has the feature: full_crypto, sp-io has the feature: disable_panic_handler. And there are a lot more crates with their own features which are critical to downstream dependencies. None of these features are included in the umbrella crate. There needs to be script which pulls in all of the features of the individual crates in to the umbrella crate.

3. Umbrella crate has a very weird semver on crates.io

This is to me, perhaps the most important issue. The latest umbrella crate has a version v0.7.0 which corresponds to stable2409. But this versioning strategy is uncorrellated with the actual github versioning which would be 1.16.0. It's important that we use the same version everywhere to avoid confusion and dependency hell for downstream users. The umbrella crate should follow the official github release versioning. This would mean publishing the correct version for the polkadot-sdk crate that corresponds with it's github tag. Eg instead of 0.8.0 for stable2412 this should be v1.17.0.

This also allows for publishing patch versions, in between stable releases.

@aurexav
Copy link
Contributor

aurexav commented Dec 24, 2024

@boundless-forest
Copy link
Contributor

I strongly support 3.

It's very confusing to understand the relationship between the various version numbers and the polkadot-sdk version when examining a project.

For example, can you tell me which version of polkadot-sdk is used for this project if you see the following dependencies in the Cargo.toml file:

sc-network-common = { version = "0.44.0" }
sc-network-sync = { version = "0.44.0" }
sp-api = { version = "34.0.0", default-features = false }
sp-database = { version = "10.0.0" }
sp-genesis-builder = { version = "0.15.1", default-features = false }

Currently, I prefer using the git dependency to specify the polkadot-sdk version. It's more explicit and easier to understand.

@ggwpez
Copy link
Member

ggwpez commented Jan 2, 2025

About 3:

We tried to get rid of the numbers (like 1.16), but it is not possible since many systems expect version numbers. So now we are stuck with those legacy numbers and the new naming schema (stableYYMM).
Aligning them seems like a good idea, although we cannot do it in a way that respects semver, since the umbrella crate will mostly always have breaking changes.
@EgorPopelyaev @Morganamilo, ideas? We can bump major on that thing, but then it would be out of sync with the GitHub release number again...

2:

There needs to be script which pulls in all of the features of the individual crates in to the umbrella crate.

Yea we can do this.

1:

umbrella crate only to discover it is incompatible with the macros in

These macros are buggy then and should know where to find their dependencies, should be able to fix it.

@ggwpez
Copy link
Member

ggwpez commented Jan 2, 2025

We could also go the other way and do 24.09.0 as version number for the umbrella crate to link it up to stable2409. The we could also still do patch releases.

@seunlanlege
Copy link
Contributor Author

We could also go the other way and do 24.09.0 as version number for the umbrella crate to link it up to stable2409. The we could also still do patch releases.

The issue is, this versioning doesn't make sense for 3rd party crates that depend on polkadot-sdk. I think we need a solution that serves both.

3rd party crates should be able to communicate what Polkadot-sdk version they support and still make minor and patch releases

@gui1117
Copy link
Contributor

gui1117 commented Jan 16, 2025

Versioning the crate with 1.16 would be inconsistent with semver traditionally used in crates.

Maybe the cleanest is 2412.0.0
Then for 2412-1, we can use 2412.1.0

@seunlanlege
Copy link
Contributor Author

Then for 2412-1, we can use 2412.1.0

But these are supposed to be patch releases. Minor version bumps are for breaking changes

@seunlanlege
Copy link
Contributor Author

seunlanlege commented Jan 22, 2025

Any updates with this? Other polkadot-sdk library maintainers are also making the same complaints. I'd say this issue is very existential for the success of the polkadot-sdk project.

polkadot-evm/frontier#1560 (comment)

For some reason, stable2409-3 which is really v1.16.3 was published as polkadot-sdk v0.9.0 whose previous version v0.7.0. Shouldn't the update to the umbrella crate have been a patch version?

This is system is absolutely untennable. It's important to get this right because git dependencies cannot benefit from the stable releases and their subsequent patch versions without requiring ecosystem libraries to update as well.

Recommended reading

https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#tilde-requirements

@ggwpez
Copy link
Member

ggwpez commented Jan 22, 2025

We brought this up yesterday in the OpenDev call, outcome was something like 2412.0.0, 202411.0.0 or similar.

But these are supposed to be patch releases. Minor version bumps are for breaking changes

Minor version changes are for additions. They dont break stuff.

Maybe the cleanest is 2412.0.0
Then for 2412-1, we can use 2412.1.0

Sounds good to me.

@ggwpez
Copy link
Member

ggwpez commented Jan 22, 2025

👀 Version Poll 📜

Poll for parachain devs and other devs downstream of the SDK. What versioning schema do you want for the Umbrella crate?
Example for the stable2412 release; please post a comment with your Project name and an option:

  • A) 2412.0.0 / 202412.0.0
  • B) 17.1.x
  • C) 1.17.0
  • D) Other (please specify)

@seunlanlege
Copy link
Contributor Author

seunlanlege commented Jan 22, 2025

Project name: Hyperbridge

Strongly prefer B) 17.1.x. It Aligns well with the default cargo semver semantics and is familiar to everyone switching from git dependencies.

@aurexav
Copy link
Contributor

aurexav commented Jan 22, 2025

Option A makes sense to me immediately; I understand its meaning without needing to think further. In contrast, the other options require a moment of reflection before I can fully grasp their meaning.

@gui1117
Copy link
Contributor

gui1117 commented Jan 23, 2025

What does 1 mean in 17.1? Would there be 17.2 for the release of polkadot 2.17.0?

If we don't expect more than 99 minor versions, we can also write 117.x.x

@blakebyrnes
Copy link

Prefer something that follows semver (B or C). The current versioning has recently bumped major versions for frame_system and frame_support which caused subxt, hyperbridge and other libraries to be incompatible. You end up having to maintain a fork of every project. If there's a way to only force that when there are truly breaking changes, it seems like a more sane option.

@jak-pan
Copy link

jak-pan commented Jan 23, 2025

Please use industry standard semver https://semver.org/ I have been personally asking for this for ages. I see there are other proponents. You can instantly see breaking changes propagate through packages and it's super simple to follow. If integration test breaks, you know what to do.

@conr2d
Copy link
Contributor

conr2d commented Jan 23, 2025

I completely agree with @jak-pan's point. Would it be possible to rename the existing Polkadot SDK packages and adhere to proper semantic versioning (semver) standards moving forward? While we cannot undo the history of published crates, we still have an opportunity to clean things up and establish a more maintainable versioning policy. This would make the ecosystem more predictable and easier to work with for everyone.

@seunlanlege
Copy link
Contributor Author

I completely agree with @jak-pan's point. Would it be possible to rename the existing Polkadot SDK packages and adhere to proper semantic versioning (semver) standards moving forward? While we cannot undo the history of published crates, we still have an opportunity to clean things up and establish a more maintainable versioning policy. This would make the ecosystem more predictable and easier to work with for everyone.

I second this, the umbrella crate is merely a lipstick on a pig. The underlying issue starts with the actual packages.

For those I'd be onboard with a scheme like

2412.1.x

Just because they already have impractical semver versions like 40.0.0 and we need to start from scratch.

@bkchr
Copy link
Member

bkchr commented Jan 23, 2025

Please use industry standard semver https://semver.org/ I have been personally asking for this for ages. I see there are other proponents. You can instantly see breaking changes propagate through packages and it's super simple to follow. If integration test breaks, you know what to do.

We already do so. Maybe we are bumping some things too fast, but tooling is there that checks which bumps are required etc.

The umbrella crate on the other side gives the user the opportunity to use easily a specific version of the polkadot sdk they want to use. By using something like 2412.0.0 gives the user the opportunity to easily specify the version. Also it enables us to push patch releases quite easily.

For downstream users it also doesn't mean that they need to use the umbrella crate everywhere, but they could do the following:

[dependencies]
polkadot-sdk = { version = "202412.0.0" }
sp-io = "*"

Because polkadot-sdk will dictate the package versions, you can just use * as version for sp-io and it will select the same version as somewhere deep inside polkadot-sdk. The same applies to every package. IMO this would greatly simplify the downstream versioning and would make the requirement on external tools for determining the package version obsolete.

@ggwpez
Copy link
Member

ggwpez commented Jan 23, 2025

Please use industry standard semver [...]

The issue with the Umbrella crate is that it pulls in all other crates. That means that as soon as any of its dependencies has a breaking change, it will also need a major bump. Otherwise it could silently break builds if we pull that dependency update in and only do a minor bump.

Now on every new stable release there is probably one create that had a major bump, therefore the umbrella will pretty much guaranteed also always be major bumped...

Not sure what else you expected from SemVer, it is what got us into this situation in the first place.

The underlying issue starts with the actual packages. For those I'd be onboard with a scheme like
2412.1.x
Just because they already have impractical semver versions like 40.0.0 and we need to start from scratch.

Yes sounds reasonable 👍

@drewstone
Copy link
Contributor

I support A or B. Don't have huge skin in the game just want something that can remain sensible for the future.

@kayabaNerve
Copy link

I support 2412.minor.patch, with minor releases not including breaking changes. Any breaking changes should be delayed until the next major release (on the regular cycle), even if immediately ready, as per semver.

@ggwpez
Copy link
Member

ggwpez commented Jan 28, 2025

Prefer something that follows semver (B or C). The current versioning has recently bumped major versions for frame_system and frame_support which caused subxt, hyperbridge and other libraries to be incompatible. You end up having to maintain a fork of every project. If there's a way to only force that when there are truly breaking changes, it seems like a more sane option.

@blakebyrnes can you please elaborate on this? We switched to SemVer to precisely mark breaking changes.
Every time when there is a breaking change in the code as defined by cargo, we bump the major. For example when there is a breaking change in sp-io, then all crates that depend on sp-io also need to be major bumped. This means that pretty much everything will be major bumped to signal that incompatibility.

"incompatibility" here means anything that could result in build failure for someone using the library (excluding upgrading Rust versions).
Every three month when we do a new stable release, many crates will have a major bump for the reason stated above. This is correct SemVer behaviour, we have tooling to check that no incompatibility slips through (although it is not always working currently).

Now in your case it seems that you actually dont want SemVer, but some more general means of "compatibility". Basically something like "not too much changed to break my particular usecase". This is definitely outside of the scope of what SemVer is able to communicate. SemVer is really dumb in this sense. It is just "could something have broken from this update", not "does anyone actually need to care about the things we changed".
I am not sure what to best do in that case besides explicitly having a list that marks all major versions that you are compatible with and extending that list every time that a new major version is released. After all, there could have been a change that broke your use-case. That is all the SemVer major tells you.

@jak-pan
Copy link

jak-pan commented Jan 29, 2025

... For example when there is a breaking change in sp-io, then all crates that depend on sp-io also need to be major bumped. This means that pretty much everything will be major bumped to signal that incompatibility.

This is wrong. SemVer is supposed to show breaking changes to public APIs in the library or package itself. It should not be bumped when library that it depends on changes major version without any change to the to the library that uses it. This way it would indeed be useless.

Yes, if you release new version of "umbrella/polkadot-sdk" which is just a wrapper for your other crates and any of the crates inside change major version (breaking change to it's APIs) then you should bump major version upon this release. But this should definitely not be bumped if some internal dependency changed. Nor should all the crates depending on breaking crate be bumped. That should be avoided at all costs.

@gui1117
Copy link
Contributor

gui1117 commented Jan 29, 2025

... For example when there is a breaking change in sp-io, then all crates that depend on sp-io also need to be major bumped. This means that pretty much everything will be major bumped to signal that incompatibility.

This is wrong. SemVer is supposed to show breaking changes to public APIs in the library or package itself. It should not be bumped when library that it depends on changes major version without any change to the to the library that uses it. This way it would indeed be useless.

Most of primitives and frame crates builds upon their dependency, most of them expose the behavior of their dependency in their public API.

I haven't tried but I doubt you can compile a runtime with 2 version of sp-io because 2 pallets are using different version in they minor release.

@acatangiu
Copy link
Contributor

I haven't tried but I doubt you can compile a runtime with 2 version of sp-io because 2 pallets are using different version in they minor release.

I tried this. It doesn't work.

This is wrong. SemVer is supposed to show breaking changes to public APIs in the library or package itself. It should not be bumped when library that it depends on changes major version without any change to the to the library that uses it. This way it would indeed be useless.

I won't comment on what is "pure" SemVer and what is not, but for our case:

  • changes to any pub-exposed primitive in a crate is a change to the crate APIs, meaning major (or at least minor) bump,
  • since that primitive struct/object/type/trait/etc could be used by the dependant crates (further up the stack), they have no choice but to also inherit the major/minor bump,
  • even if, say, my crate uses only struct RuntimeAllocator from sp-io crate, and this struct doesn't change in any way between major version updates of sp-io, cargo/rustc still complains the struct is incompatible - I believe this is because the actual full type-name used by the compiler is derived from the (crate-name, crate-version, type-name) tuple, so even if the type has not practically changed, the compiler will complain about compatibility.

So afaict we really have no choice but to cascade bumps upwards from lower-level deps.

@ggwpez
Copy link
Member

ggwpez commented Jan 29, 2025

changes to any pub-exposed primitive in a crate is a change to the crate APIs, meaning major (or at least minor) bump,

Yes and not just stuff that is directly in a public API type but also behavioural changes thereof. People love to depend on undocumented behaviour.

But this should definitely not be bumped if some internal dependency changed.

But how does automation know if it is just internal or changes the behaviour of the crate in a breaking manner?

For example, when sp-io has a major bump; can we just pull that in with a patch into sp-storage? Maybe it breaks how sp-storage behaves or maybe it does not. Maybe. But maybe is not an answer that we can automate for 400 crates in the CI on every merge request. So we went with Yes so far.

Nor should all the crates depending on breaking crate be bumped. That should be avoided at all costs.

🫠 Then we should have talked it first through, since we interpreted the calls for SemVer as it is defined in the standard. Bumping major in SemVer just tells you that something could have broken, so we need to do it for pretty much every new stable release.

@blakebyrnes
Copy link

blakebyrnes commented Jan 29, 2025

@blakebyrnes can you please elaborate on this? We switched to SemVer to precisely mark breaking changes.
Every time when there is a breaking change in the code as defined by cargo, we bump the major. For example when there is a breaking change in sp-io, then all crates that depend on sp-io also need to be major bumped. This means that pretty much everything will be major bumped to signal that incompatibility.

Maybe my thoughts on this were/are too superficial. I would have thought (as per a few other commenters) that a semver major should represent a breaking api change in that specific package. But I do see how the rust pattern of re-exporting crates and preludes seem to make that nearly impossible.

I think if I were to give this a lot of thought/time, I might go down a few lines of thinking:

  1. Are there crates that could be consolidated?
  2. Can some patterns like preludes be removed in one version and moved to a new type of prelude style crate (like umbrella)?
  3. Like item 2, can re-exporting of sub crates be removed
  4. Does test coverage get you to a place where you can have coverage of public apis enough to confidently say the semver hasn't changed?
  5. Would it help to have a sample project (like the templates) that also build a subxt client and/or use an external pallet system like orml or hyperbridge? That could at least serve as a guiding point for the pain points (if it's not already something that exists).

Wish I could lend some more concrete suggestions, but I do see the scope of the challenge.

@kianenigma
Copy link
Contributor

  1. The matter of versioning for major crates that are meant to be "simple" to use is important, I listed all the crates that I think should have a clear and easy version number here: Align omni-node and polkadot-parachain versions #7367 (review). For now I can say all of these will have better versions by stable-202506 release, not sure what else is there.
  2. Feature propagation is a known issue and tracked here: Support for Crate-Specific Features via Umbrella Dependency #5934
  3. As noted above, I want to double assert that, excluding using polkadot-sdk umbrella, everything else is being updated now based on semver. If e.g. sp-io remains un-chaned from stable-x to stable-y, its version remains the same. Else, it will receive the right major/minor/patch.

@ggwpez
Copy link
Member

ggwpez commented Jan 30, 2025

sp-io = "*"

That does not work for crates that should be published, cargo errors in that case if you use a * dependency:

error: failed to publish to registry at https://crates.io

Caused by:
  the remote server responded with an error (status 400 Bad Request): wildcard (`*`) dependency constraints are not allowed on crates.io. Crate with this problem: `hex` See https://doc.rust-lang.org/cargo/faq.html#can-libraries-use--as-a-version-for-their-dependencies for more information

Just because they already have impractical semver versions like 40.0.0 and we need to start from scratch.

I think we could try to set all SDK crate versions to the same in the next release. This would mean that we do major bumps for all crates on every stable release (3 months), but it also means that since all crates would have the same versioning schema (eg 2503.0.0) that it is obvious to what release they relate.

I would like to setup some call to finalize on something here, as just in GH discussions it is a bit difficult for me to get a feeling for how "on-board" you would be with any outcome.

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

No branches or pull requests