node: Remove default feature
#45
Conversation
tcharding
force-pushed
the
01-22-fix-core-28
branch
from
899189e to
896156b
Compare
tcharding
force-pushed
the
01-22-fix-core-28
branch
from
896156b to
ccac13d
Compare
Every single use statement in `client_versions` has an `allow` attribute; this is too noisy.
If no features are enabled then a few things happen: - `bitcoind` from `$PATH` is used. - We assume Core version 28 This means that in order to make `--no-default-features` build (which is a reasonable expectation) there are implicit requirements on the environment - this is bad but I see no other solution ATM. Improve the code comments, make the attributes uniform, and add missing version to the no-feature-enabled logic. Document the `bitcoind` version requirements in the README.
The `default` feature is problematic because it overrides `--features=xyz` if `--no-default-features` is used. Remove it.
tcharding
force-pushed
the
01-22-fix-core-28
branch
from
ccac13d to
8d08689
Compare
|
I'm interested to see if this one gets through your CI please @apoelstra. |
|
Sure, will run this through local CI and ack it. But I'm not convinced that We maybe also ought to warn if multiple features are enabled. |
hmm. You could be right, its (By "has to" I mean, its annoying if it doesn't - I remember we had that somewhere when I first started in this org and it bit me a bunch.) |
|
Yeah, true, we had that with the But this is a somewhat-obscure leaf crate, which will make it less annoying, and I also think that users would like to be forced to choose a Core version rather than unknowingly going with "the default" and finding that this version changes out from under them. |
The
defaultfeature innodeoverides an explicit version feature. This is a footgun.Remove the
defaultfeature fromnode.Note
The features in the
nodecrate are not 'normal'. Even so I personally maintain that--all-featuresand--no-default-featuresshould build and be footgun free. With this PR merged there is a remaining footgun but it relates to running unit tests locally instead of in using the crate as a dependency - that's not a win but its an improvement.Fix: #40