rust-amplify
fix alloc/std features
While trying to fix the build of rust-aluvm with only the alloc feature (cargo check --workspace --no-default-features --features alloc) I've noticed there's quite a mess regarding std and alloc features. For example there are crates like baid64 that are not defining these features (therefore forcing their dependents to import std) or amplify that exports the confinement module only if alloc is activated (causing builds with only std to fail).
This PR fixes imports of std and alloc features in a way that now amplify doesn't import std if we specify --no-default-features --features alloc and viceversa.
This change though:
- #[cfg(feature = "std")]
+ #[cfg(all(feature = "std", not(feature = "alloc")))]
can work only if the std and alloc are mutually exclusive and if dependencies that use the alloc feature do not have other dependencies importing amplify with the std feature.
I can fix these issues but first I need to understand what's the desired result. My proposal is to make all dependencies have std and alloc features, make them exclusive and to expose everything in both std and alloc version, @dr-orlovsky let me know your thoughts about this
My proposal is to make all dependencies have std and alloc features, make them exclusive and to expose everything in both std and alloc version
Yes, I do agree
@dr-orlovsky I'm adding/fixing the std and alloc features in an exclusive way (i.e. you cannot build without at least one of the features and you cannot build with both features) as agreed here. I've encountered an unexpected issue though. This way, if you try to build with --all-features the build fails, because of the check that gives an error when building with both std and alloc.
If you think this is not an issue I'll just document this on the README and/or on the generated documentation and fix the CI jobs where --all-features is used by changing it to --features all. If instead you think this is an issue I can make changes in order to make compilation with both features possible (importing from std in that case).
Another, maybe cleaner, solution I see would be to drop the alloc feature, so when the user doesn't provide the std feature we import from alloc.
Could you please tell me what you think is best?
I'm adding/fixing the std and alloc features in an exclusive way
I just understood that this is a breaking change - the crates downstream may stop from compiling with that change.
Overall, from my experience changing std/allocs is a PITA breaking everything - and we risk to end up spending >week on settling the things. I am not sure whether this is a priority at this stage: RGB doesn't use no-std yet and it won't be needed until we will develop apps for hardware wallets - and first we need to get v0.11 released.
I think these changes are not worth the time we will need to invest into it.
@dr-orlovsky Actually I have already fixed almost everything, I don't think it will take me weeks to complete this. For us this is important so please let me know what proposal you prefer (i.e. not compiling with --all-features or allowing both features to be active or removing the alloc feature) so I can complete this task
I am not talking about completing this. I am talking about fixing compilation of hundreds of crates depending on this one :)
I have no answer. I need to invest hours into checking all options against downstream crates and find the least non-inteusive
I am not talking about completing this. I am talking about fixing compilation of hundreds of crates depending on this one :)
I was talking about fixing compilation of ~20 crates, all crates that are needed for RGB to work, from crates in RGB-WG, BP-WG, UBIDECO and rust-amplify. It's not requiring so many changes and I think this is worth it
I have no answer. I need to invest hours into checking all options against downstream crates and find the least non-inteusive
All options are not so intrusive, so please choose the one that seems cleaner to you and I'll do it. You don't need to worry about the work
Rust-amplify is used not just by RGB. We have to fix all of the crates.
This change also should go as a new 5.0 release - and there are a lot of work to do for its proper support downstream - and certainly this won't be scheduled until RGB 0.11 is released, since before the release we merge only critical security bugfixes.
I do mot understand why this is of any importance before the 0.11 release.
Rust-amplify is used not just by RGB. We have to fix all of the crates.
I see it has a lot of dependants https://crates.io/crates/amplify/reverse_dependencies but most of those crates are archived/abandoned. Also I don't understand what do you mean by "fix the crates". Currently the alloc feature is not working and is therefore unused. We will not rename the std feature so I don't see which changes would be needed on the dependants. The only change it might be needed is updating the MSRV in case we agree putting an higher one in amplify-derive, but that doesn't seem like a hard thing to do.
This change also should go as a new 5.0 release - and there are a lot of work to do for its proper support downstream - and certainly this won't be scheduled until RGB 0.11 is released, since before the release we merge only critical security bugfixes.
Not merging other stuff before a 0.11 release was your proposal that we haven't discussed. It's not an issue anyway, we can merge these PRs after te 0.11 release and make them part of the next one. This is important for us so we need this to happen and the version number doesn't matter.
but most of those crates are archived/abandoned
Even a single non-abandoned dependency from outside of our crates is already enough not to introduce breaking changes and strictly follow semantic versioning. For instance, there is https://crates.io/crates/tor-config with >100k of users, which is maintained, as well as dozens of others.
Currently the alloc feature is not working and is therefore unused.
Rust dependencies work in a complex way and it is easy to break things. Thus, easier to just do that in major version upgrade with no risk.
Closing in favor of #270 which is much less invasive and doesn't break anything downstream.
I have published it as v4.8.0 so dependencies may still decide whether to upgrade. In case of any breaks reporting I will yank it out.