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.

Sign up for GitHub

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

Jump to bottom

Include and test node #53

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Include and test node #53

wants to merge 4 commits into from
+1,901 −70

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Feb 5, 2025 •
edited
Loading

Currently we have a job for each supported version of Bitcoin Core and in it we run the integration tests.

Note also that node is currently not a member of the workspace. So it misses out on all the benefits of run_task (e.g., linting).

However node cannot be directly run using run_task because of its unusual features but since we already have the Integration job set up to manage these features we can just run the node tests from there also. This has the added advantage that the same downloaded Bitcoin Core binary will be used (I hope).

Add a script and call it from the Integration job. In the script first run the node tests and then run the integration tests.

@tcharding tcharding mentioned this pull request Feb 5, 2025
Merged
@tcharding
Copy link
Member Author

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

@tcharding
Copy link
Member Author

BOOM! Mad, no further improvements needed.

@tcharding tcharding mentioned this pull request Feb 5, 2025
Draft
apoelstra
apoelstra previously approved these changes Feb 5, 2025
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 84ac230; successfully ran local tests

@apoelstra
Copy link
Member

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

@apoelstra
Copy link
Member

apoelstra commented Feb 5, 2025 •
edited
Loading

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

@apoelstra apoelstra dismissed their stale review February 5, 2025 16:01

change does not do what I expected to local CI

@tcharding
Copy link
Member Author

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 tcharding force-pushed the 02-05-no-exclude-node branch from 84ac230 to 81101c7 Compare February 5, 2025 20:17
@tcharding
Copy link
Member Author

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
Copy link
Member Author

Ah good, my local test command is now failing.

BOOM! Mad, no further improvements needed.

Face palm.

@tcharding tcharding marked this pull request as draft February 5, 2025 20:20
@apoelstra
Copy link
Member

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 tcharding force-pushed the 02-05-no-exclude-node branch 2 times, most recently from f2af300 to 313ef9d Compare February 6, 2025 02:59
@tcharding tcharding mentioned this pull request Feb 6, 2025
Merged
@tcharding tcharding force-pushed the 02-05-no-exclude-node branch 3 times, most recently from 8a0e63d to d276794 Compare February 6, 2025 03:29
@tcharding tcharding marked this pull request as ready for review February 6, 2025 03:31
@tcharding tcharding force-pushed the 02-05-no-exclude-node branch from d276794 to 68c6087 Compare February 6, 2025 03:38
@tcharding
Copy link
Member Author

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

tcharding added a commit that referenced this pull request Feb 6, 2025
@tcharding
Merge #54: Clean up the node manifest
8f86bc1 
9ead0fa Make anyhow an optional build-dependency (Tobin C. Harding)
4016843 Put dep on single line (Tobin C. Harding)

Pull request description:

  Quick cleanup to the `node` manifest. Done while working on #53

ACKs for top commit:
  apoelstra:
    ACK 9ead0fa; successfully ran local tests

Tree-SHA512: d5556518779eae439858c656583a426b36d2d96753c21157955564d67482bfd251696498417b8a8e21d95f87e07dd71b9b3770f5dc4fb27679097c57f929c2b1
@tcharding
Run the formatter
af10c00 
In preparation for including `node` in the workspace run the formatter
on `node`.
@tcharding
Fix rustdoc format
0b00390 
rustdoc expects to be on an item. Make the docs on `tmpdir` conform to
rustdoc format.
@tcharding tcharding force-pushed the 02-05-no-exclude-node branch 3 times, most recently from 5e69dc4 to 13d2815 Compare February 6, 2025 22:24
@tcharding
Copy link
Member Author

tcharding commented Feb 6, 2025 •
edited
Loading

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

@tcharding
Fix feature guard on v19 test
f09cd50 
The `getbalances` RPC method is currently being feature guarded on
`not(download)`. I'm not sure why but it seems unnecessary.
@tcharding tcharding force-pushed the 02-05-no-exclude-node branch 2 times, most recently from bdefcaa to 17b94df Compare February 6, 2025 22:44
@tcharding
Copy link
Member Author

I used the changes to update-lock-files.sh to update the lock files in #49.

apoelstra
apoelstra reviewed Feb 7, 2025
View reviewed changes
Cargo-minimal.lock Outdated

[[package]]
name = "regex-syntax"
version = "0.8.5"
Copy link
Member

Choose a reason for hiding this comment

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

In 17b94df:

regex-syntax 0.8.5 does not work with Rust 1.63. (Requires 1.65.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn, MSRV is not being checked currently. Back to the drawing board. Thanks

@tcharding
Run node tests
b657288 
Currently we have a job for each supported version of Bitcoin Core and
in it we run the integration tests.

Note also that `node` is currently not a member of the workspace. So it
misses out on all the benefits of `run_task` (e.g., linting).

However `node` cannot be directly run using `run_task` because of its
unusual features - it requires a version feature to build.

Add a script to lint and one to run tests then call these from CI.

While we are at it update the justfile now that `node` can be linted and
formatted as part of workspace commands. Also update the
`update-lock-files` script.

This PR adds a ton of dependencies to the lock file. Run `cargo` with
`-Z minimal-versions` to try and get a minimal lock file that works.
@tcharding tcharding force-pushed the 02-05-no-exclude-node branch from 17b94df to b657288 Compare February 10, 2025 03:09
@tcharding tcharding marked this pull request as draft February 10, 2025 03:21
@tcharding
Copy link
Member Author

I played with this a bit today but its super boring. I rekon I'll merge #55 tomorrow and then merge this and cut the release. Lets punt this PR to the next release.

@apoelstra
Copy link
Member

Ok. I will just utACK stuff then.

@tcharding
Copy link
Member Author

After sleeping on this I think it is actually a big deal because we (I) claim that the node crate is MSRV 1.63 and if its not being tested the the claim is worthless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoelstra apoelstra apoelstra left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tcharding @apoelstra