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

Cumulative develop branch update #73

Merged
merged 11 commits into from
Sep 14, 2021
Merged

Cumulative develop branch update #73

merged 11 commits into from
Sep 14, 2021

Conversation

sorpaas
Copy link
Member

@sorpaas sorpaas commented Jun 18, 2021

No description provided.

sorpaas and others added 11 commits May 18, 2021 23:46
* Fix styling

* Run cargo fmt

* Bump core version to 0.2.2-dev

* More version bumps
…text (#48)

* Documents feature flags in README and add support for lazy-static-context

* Fix lazy static context feature flag

* Add compile error

* Bump version to 0.6.0-dev

* More all -> any feature flag fixes

* Fix clippy feature flags
* Fixed benches to use new parse functions

* Fixed building for no_std
* Release v0.6.0

* Fix version dep

* Fix top-level package version dep
* Fix no_std builds attempt 2

- Turn off default features for core in gen and genmult
- Use serde-json-core (no_std support) in dev-dependencies

Relates to #71

* Use resolver="2"

Allows us to use serde_json in dev-dependencies and compile for no_std:

https://blog.rust-lang.org/2021/03/25/Rust-1.51.0.html#cargos-new-feature-resolver

* revert unnecessary changes
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

LGTM but I would really appreciate if features would stay additive.

Comment on lines +40 to +41
#[cfg(all(feature = "static-context", feature = "lazy-static-context"))]
compile_error!("Should only enable one of static-context or lazy-static-context");
Copy link
Member

Choose a reason for hiding this comment

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

Can't we get away with this? Making features non-additive makes life hard for downstream as it can lead to incompatible dependency trees. Maybe just automatically assume lazy static when static-context isn't set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Those two feature flags are never meant to be used together. If that happens, the combination is meaningless and it's a config error of the packages using it.

Copy link
Member

Choose a reason for hiding this comment

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

The thing is that this "meaningless" combination isn't the fault of any packages using this crate. Imagine this dependency graph ( X -> Y means X depends on Y):

A -> B
A -> C
B -> libsecp256k1(static_context)
C -> libsecp256k1(lazy_static_context)

The way that feature unification works in cargo is that a single version of libsecp256k1 is included in the dependency tree with both features enabled. It isn't the fault of A that its dependency use incompatible features.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. We need to choose which overwrites which though. I'll take a look (mostly whether lazy_static works good enough on no_std) to resolve this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like an easy decision -- static_context doesn't have additional dependencies, while lazy_static_context introduces lazy_static. So let's make lazy_static_context overwrites static_context.

@sorpaas
Copy link
Member Author

sorpaas commented Sep 14, 2021

Please don't merge this yet. We need to enable merge commit for this repo and merge it that way.

(Now merge-able!)

@sorpaas sorpaas merged commit 54c7ccd into master Sep 14, 2021
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