Introduce more optional features in ethcore by udoprog · Pull Request #9020 · openethereum/parity-ethereum

udoprog · 2018-06-30T15:28:45Z

This introduces optional features to ethcore. If they are not enabled, they leave out the entire hyper/tokio ecosystem which is a lot of dependencies which are not needed when doing things like smart contract testing (e.g. parables).

This is cargo tree --verbose in ./ethcore before and after the change.

Before:

cargo tree --verbose | wc -l
691

After:

cargo tree --verbose | wc -l
493

parity-cla-bot · 2018-06-30T15:28:48Z

It looks like @udoprog signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

5chdn · 2018-06-30T19:26:27Z

error: Could not compile parity.

udoprog · 2018-06-30T20:14:40Z

@5chdn Ah, tests are run with --no-default-features. So all the things provided by dependencies through features have to be conditionally supported as well?

tomaka · 2018-06-30T20:27:26Z

So all the things provided by dependencies through features have to be conditionally supported as well?

I'm not sure if I understand your comment correctly.
If compilation fails when you disable a feature, then it shouldn't be a feature in the first place.

udoprog · 2018-06-30T20:48:17Z

@tomaka well... yeah :). That made it click. They should be part of the dependency, not the local [features] for the parity app.

dvdplm

While I totally understand the ambition to reduce ethcore compile times and complexity, I am not sure I agree that this is the right approach. It seems to add a lot of visual clutter and adding a barrier to reason about the code – "is this code compiled for the binary used?" is not something I'd like to ponder while fixing user bugs.
At the very least, the various features need good docs that explain when/why they're used.

tomaka · 2018-07-02T09:29:05Z

For what it's worth, this feature is desirable for #7915 because hyper and tokio don't work when inside a browser.

udoprog · 2018-07-02T12:30:06Z

@dvdplm Hey!

I am not sure I agree that this is the right approach

Increasing modularization seems to be the right long term approach. Features are more like a way to get it done right now, and they can be removed as things are broken up into more composable traits, types, and crates. But apart from not being as visible as a dependency, a missing feature causes a compile-time error similar to a missing dependency.

"is this code compiled for the binary used?"`

Note that the features introduced here are not gated in the binary (thanks due to the comment by @tomaka). You would get a compile time error if you try to use a type or function that is feature-gated in ethcore. It's similar to if you were trying to use serde for a dependency without enabling the feature.

tomaka

The intent is good, just some minor issues.

tomaka · 2018-06-30T16:08:04Z

 }

 impl Miner {
+	#[cfg(feature = "work-notify")]


Nit: this line should be under the /// comment.

tomaka · 2018-06-30T16:14:27Z

+			let _stratum = stratum;
+		}
+
 		Ok(())


IMO, an error should be returned if work-notify is disabled.

In that case, maybe we should just gate the pub fn register function for a compile-time error?

Since the stratum feature is mandatory in the Parity client, I guess it's indeed better to disable the function entirely.

Done, thanks!

tomaka · 2018-07-03T12:40:12Z

 pub enum GasPricer {
 	/// A fixed gas price in terms of Wei - always the argument given.
 	Fixed(U256),
+	#[cfg(feature = "price-info")]


Nit: should be under the ///.

tomaka · 2018-07-03T12:40:18Z

 }

 impl GasPricer {
+	#[cfg(feature = "price-info")]


Nit: should be under the ///.

Fixed! ... Thanks!

dvdplm · 2018-07-03T14:34:40Z

@udoprog Ok, I rest my case! :)

…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: Fixes for misbehavior reporting in AuthorityRound (openethereum#8998) A last bunch of txqueue performance optimizations (openethereum#9024) reduce number of constraints for triedb types (openethereum#9043) bump fs-swap to 0.2.3 so it is compatible with osx 10.11 again (openethereum#9050) Recursive test (openethereum#9042) Introduce more optional features in ethcore (openethereum#9020) Update ETSC bootnodes (openethereum#9038) Optimize pending transactions filter (openethereum#9026) eip160/eip161 spec: u64 -> BlockNumber (openethereum#9044)

tomusdrw · 2018-07-12T08:51:23Z

-			WorkPoster::new(&cmd.miner_extras.work_notify, fetch.clone(), event_loop.remote())
-		));
+
+	#[cfg(feature = "work-notify")]


That's never enabled. main Cargo.toml doesn't have that feature.

udoprog mentioned this pull request Jun 30, 2018

Make work-notify an optional feature #9018

Closed

5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. M0-build 🏗 Building and build system. M5-dependencies 🖇 Dependencies. labels Jun 30, 2018

5chdn added this to the 1.12 milestone Jun 30, 2018

udoprog force-pushed the more-optional-features branch from 564d801 to 780a2c8 Compare June 30, 2018 20:48

dvdplm reviewed Jul 2, 2018

View reviewed changes

tomaka suggested changes Jul 3, 2018

View reviewed changes

5chdn added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 3, 2018

udoprog force-pushed the more-optional-features branch from 780a2c8 to ec39246 Compare July 3, 2018 13:22

John-John Tedro and others added 5 commits July 3, 2018 18:04

Make work-notify an optional feature

dcf1cfa

More optional ethcore features: price-info, stratum

3499999

Make ethcore features part of dependency instead of local features

5a7396c

Put cfg gate in right location

9d8cdf1

Feature gate register function on work-notify

08d0e6f

udoprog force-pushed the more-optional-features branch from ec39246 to 08d0e6f Compare July 3, 2018 16:07

tomaka approved these changes Jul 3, 2018

View reviewed changes

dvdplm approved these changes Jul 4, 2018

View reviewed changes

5chdn added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jul 5, 2018

5chdn merged commit 71bbcd5 into openethereum:master Jul 5, 2018

andresilva mentioned this pull request Jul 5, 2018

bump fs-swap to 0.2.3 so it is compatible with osx 10.11 again #9050

Merged

udoprog deleted the more-optional-features branch July 5, 2018 14:38

tomusdrw mentioned this pull request Jul 12, 2018

Optional ethcore features should be enabled by default #9103

Closed

tomusdrw reviewed Jul 12, 2018

View reviewed changes

Conversation

udoprog commented Jun 30, 2018

Uh oh!

parity-cla-bot commented Jun 30, 2018

Uh oh!

5chdn commented Jun 30, 2018

Uh oh!

udoprog commented Jun 30, 2018

Uh oh!

tomaka commented Jun 30, 2018

Uh oh!

udoprog commented Jun 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

tomaka commented Jul 2, 2018

Uh oh!

udoprog commented Jul 2, 2018

Uh oh!

tomaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvdplm commented Jul 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

udoprog commented Jun 30, 2018 •

edited

Loading