Include node in workspace

[tcharding]

Currently we do not include the node crate in the workspace because run_task did not support testing it.

However run_task has been patched. Additionally we discovered that we were not testing the docs with --all-features (or any code for that matter other than linting). Fixed in:

rust-bitcoin/rust-bitcoin-maintainer-tools#20

Which this patch uses.

Include node in the workspace. Configure it to work with the newly patched run_task. Add a extra_tests.sh script that runs the node tests.

In order to get MSRV building we make all deps use default-features = false then explicitly enable just what we need.

Also change the feature guarding in the build script so that --all-features works. This is already supported in the main crate code since the highest version feature overrides the lower ones.

[tcharding]

This is mainly to see what happens. I'll come back to debug this.

[tcharding]

BOOM! Mad, no further improvements needed.

[apoelstra]

ACK 84ac230; successfully ran local tests

[apoelstra]

Lemme dig into this -- did not expect that local CI run to pass.

[apoelstra]

Ah, it looks like you removed node from the exclude list but did not put it into members.

change does not do what I expected to local CI

[tcharding]

did not expect that local CI run to pass.

Neither did I.

it looks like you removed node from the exclude list but did not put it into members.

lol, its hard to get good help.

[tcharding]

FTR what is the point of having both an explicit 'include' and an 'exclude' list - surely one is default since something must be either in the group or out of the group.

[tcharding]

Ah good, my local test command is now failing.

BOOM! Mad, no further improvements needed.

Face palm.

[apoelstra]

FTR what is the point of having both an explicit 'include' and an 'exclude' list - surely one is default since something must be either in the group or out of the group.

I think that if you have a crate in neither list, and you try to build that crate directly, it'll complain that it sees a Cargo.toml in a higher directory and isn't sure what to do.

I don't know if this is a bug or intended behavior or what.

Anyway, looks like we're getting the CI failures that we expect now.

[tcharding]

I'll come back and debug tomorrow. just lint and just fmt both run cleanly on my machine.

[tcharding]

Now includes a custom lint script. And WIN, the script found a bug in the feature gating of node.

[apoelstra]

Lol, nice, adding those redudant imports so the compiler output isn't such a mess without features.

[apoelstra]

In 84c45ba:

This does not build if you enable 0_17_1 download doc and nothing else.

It may help to draw out an ascii-art truth table in the file.

[tcharding]

This does not build if you enable 0_17_1 download doc and nothing else.

There is no reason to enable doc unless one is building the docs. But I can have another go, no sweat. Sorry this is dragging out so much.

[tcharding]

I think its correct as it is man. We are abusing the download and doc features by using not on them so as to support --all-features. Said another way, we have to have at least one not statement on main otherwise it triggers for --all-features and for this we use doc and download.

If I'm correct then it implies changing your CI, is that acceptable?

[apoelstra]

I was ok with saying "you need at least one of the version features" but not with this byzantine set of rules for which feature combinations are permissible. I can update my local CI but I haven't even gotten that far because manually understanding the feature gates takes so much work and I haven't completed it without encountering something surprising.

At the very least, if we don't want certain combinations to work then we should have an explicit compile_error about them.

[tcharding]

Let me try solve this at a higher level. We have a bunch of smellyness in the features. doc is unusual and the feature ones are unusual in multiple ways (require at least one and enable all versions lower).

[tcharding]

Hoorah!! We got there. I removed the doc feature all together. The fail was not noticing this when I introduced rustdoc-args = ["--cfg", "docsrs"] in the manifest ages ago.

[tcharding]

Hopefull the docs build done by docs.rs does not run the tests during its build process. Because this crate's docs currently exist online I rekon this is true.

[apoelstra]

I've almost got this. My local CI has a cargo test --doc line which has taken a bunch of iteration, since it doesn't work with --all-features (that enables download which doesn't work inside Nix) nor does it work with no features (that triggers the compile-fail error) and obtaining a specific feature list is a bit of a trick.

There are similar lines all over the place which cover things like doctests which crate2nix/cargo don't test unless you go out of your way to explicitly specify, and they all break in some way with this crate.

[tcharding]

Holy smokes this one should not have been so hard. Here is another attempt. Please see #45 (comment) and also the additional patches before the last in this PR.

• Now cargo test should work and tests pass if you have bitcoind in your path and its version 17 (works for 18 and likely a few others too FWIW). Implies cargo test --doc works too.

• Features stack correctly in downstream crates (ie the footgun fixed in node: Remove default feature #45 is still gone)

I owe you a beer for this one.

[apoelstra]

Now cargo test should work and tests pass if you have bitcoind in your path and its version 17

LOL ok, yeah, I'll go digging around to find a copy of boost 1.75 and miniupnpc 2.2.7 and gcc 11 and whatever libc I need to work with it.

(These changes don't actually make the situation any worse -- if it's true that I need to somehow obtain and run extremely old versions of Core to make the tests pass then that's going to be a problem for me no matter what.)

[tcharding]

How about we not run node builds/tests in your CI and rely on GitHub for testing it? (That is not a smart arse comment, I searious.)

[apoelstra]

As long as node is in the workspace.members part of the manifest then I have to deal with the existence of the download feature.

If I can't build specific versions of Core then sure, I can just disable those.

[tcharding]

I don't know what to do about this now man.

[apoelstra]

I'm working on it.

[tcharding]

No stress, no rush. FTR I'm not itching to release now, that itch went away.

[tcharding]

Hey man, I'm going to go ahead and merge this. I need some time away from rust-bitcoin and this repo is the other thing I can work on without waiting on or arguing with other people. I'm totally drained right now. I just want to crawl off into a corner and write some code. Please don't take me pushing forward as any comment on you or your CI, both of which I'm happy to work with. Its the greater rust-bitcoin 'community' that has me zonked.

[apoelstra]

No problem. I can test my local CI against master while I'm getting it working.