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

Move lightning-transaction-sync tests to dedicated script #3528

Merged

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Jan 13, 2025

.. and bump its MSRV to 1.75.

Recently, rustls bumped their MSRV to v1.71. As we depend on them and don't want to continuously pin this security-critical dependency back, we have no choice left but to bump the MSRV for lightning-transaction-sync to a version >= v1.71, too.

Here, we hence move the lightning-transaction-sync tests to a dedicated script and propose to introduce a secondary MSRV of 1.75.

We chose this particular version, because:

a) it's > 1 year old
b) it provides a buffer on top of v1.71, i.e., if some crate bumped to a version > 1.71, there is a chance we don't immediately have to react again
c) it stabilized async fns in traits (see https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html), which might become handy for related (BDK) crates, which hopefully will adopt the same target.

@tnull tnull mentioned this pull request Jan 13, 2025
@tnull tnull force-pushed the 2025-01-introduce-secondary-msrv-1.75 branch 3 times, most recently from 630804b to 98abe2d Compare January 13, 2025 12:12
Copy link

codecov bot commented Jan 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.36%. Comparing base (f92c4dc) to head (b732e76).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3528      +/-   ##
==========================================
- Coverage   88.36%   88.36%   -0.01%     
==========================================
  Files         149      149              
  Lines      112875   113053     +178     
  Branches   112875   113053     +178     
==========================================
+ Hits        99742    99899     +157     
- Misses      10653    10672      +19     
- Partials     2480     2482       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheBlueMatt
Copy link
Collaborator

Grr, can you fix CI?

error: package home v0.5.11 cannot be built because it requires rustc 1.81 or newer, while the currently active rustc version is 1.63.0

@tnull tnull force-pushed the 2025-01-introduce-secondary-msrv-1.75 branch from 98abe2d to 0829353 Compare January 13, 2025 13:06
@TheBlueMatt TheBlueMatt added this to the 0.1 milestone Jan 13, 2025
@tnull
Copy link
Contributor Author

tnull commented Jan 13, 2025

Grr, can you fix CI?

error: package home v0.5.11 cannot be built because it requires rustc 1.81 or newer, while the currently active rustc version is 1.63.0

Jup, just pushed a fix.

@tnull tnull force-pushed the 2025-01-introduce-secondary-msrv-1.75 branch from 0829353 to bff52f2 Compare January 13, 2025 13:07
@TheBlueMatt
Copy link
Collaborator

TheBlueMatt commented Jan 13, 2025

CI is still failing with error: package tinystr v0.7.6 cannot be built because it requires rustc 1.67 or newer, while the currently active rustc version is 1.63.0. I believe moving lightning-transaction-sync out of the workspace will fix it.

@tnull
Copy link
Contributor Author

tnull commented Jan 13, 2025

CI is still failing with error: package tinystr v0.7.6 cannot be built because it requires rustc 1.67 or newer, while the currently active rustc version is 1.63.0. I believe moving lightning-transaction-sync out of the workspace will fix it.

Ah, missed that it was explicitly added as a dependency to no-std-check. Dropping it from there should be sufficient.

@tnull tnull force-pushed the 2025-01-introduce-secondary-msrv-1.75 branch from a43805e to b732e76 Compare January 13, 2025 19:56
@tnull
Copy link
Contributor Author

tnull commented Jan 13, 2025

Went ahead and squashed the fixups as my hope is up that this will finally pass, and I want to enable landing it whenever it does:

> git diff-tree -U2 a43805e85 b732e763b
>

@TheBlueMatt
Copy link
Collaborator

I'd kinda prefer to remove it from the workspace as well so that cargo check passes on our MSRV. Is there a reason not to?

@tnull tnull force-pushed the 2025-01-introduce-secondary-msrv-1.75 branch from b732e76 to 8a1d5ed Compare January 14, 2025 08:06
@tnull
Copy link
Contributor Author

tnull commented Jan 14, 2025

Is there a reason not to?

Well, essentially the same argument: I'd prefer to have it included in usual tests via cargo check -p ../cargo test -p ...

I'd kinda prefer to remove it from the workspace as well so that cargo check passes on our MSRV

Even without moving it out of the workspace, cargo check on MSRV works just fine for me (as any dependencies conflicting with the MSRV are hidden behind features anyways):

> rustup override set 1.63
info: override toolchain for 'rust-lightning' set to '1.63-aarch64-apple-darwin'
> cargo check
...
    Checking lightning-transaction-sync v0.0.124 (rust-lightning/lightning-transaction-sync)
...
    Finished dev [optimized + debuginfo] target(s) in 5.59s

In any case, now went ahead and included a fixup that moves it out as per your preference. Let me know if I should squash or drop it.

@tnull tnull force-pushed the 2025-01-introduce-secondary-msrv-1.75 branch 2 times, most recently from 71ed4e8 to 7ee287b Compare January 14, 2025 08:26
@tnull
Copy link
Contributor Author

tnull commented Jan 14, 2025

Screenshot 2025-01-14 at 11 36 51 👀👀👀

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

I'd prefer to leave it out of the workspace. At least personally I use the workspace on our MSRV all the time, and the ability to run cargo test --all-features without it vomiting would be nice.

.. and bump its MSRV to 1.75.

Recently, `rustls` bumped their MSRV to 1.71. As we depend on them and
don't want to continuously pin this security-critical dependency back,
we have no choice left but to bump the MSRV for
`lightning-transaction-sync` to a version >= 1.71, too.

Here, we hence move the `lightning-transaction-sync` tests to a
dedicated script and propose to introduce a secondary MSRV of 1.75.

We chose this particular version, because:

a) it's > 1 year old
b) it provides a buffer to 1.71, i.e., if some crate bumped to a version
 > 1.71, there is a chance we don't immediately have to react again
c) it
 stabilized `async fn`s in traits (see
 https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html),
 which might become handy for related (BDK) crates, which hopefully will
 adopt the same target.
@tnull tnull force-pushed the 2025-01-introduce-secondary-msrv-1.75 branch from 7ee287b to cd5b4f7 Compare January 14, 2025 13:31
@tnull
Copy link
Contributor Author

tnull commented Jan 14, 2025

I'd prefer to leave it out of the workspace. At least personally I use the workspace on our MSRV all the time, and the ability to run cargo test --all-features without it vomiting would be nice.

Alright, now squashed with the following additional changes (reverting the hunk mentioned above):

> git diff-tree -U2 7ee287bd0 cd5b4f763
diff --git a/ci/ci-tests.sh b/ci/ci-tests.sh
index 393ce18f5..f4987569f 100755
--- a/ci/ci-tests.sh
+++ b/ci/ci-tests.sh
@@ -87,7 +87,5 @@ echo -e "\n\nTesting c_bindings builds"
 # Note that because `$RUSTFLAGS` is not passed through to doctest builds we cannot selectively
 # disable doctests in `c_bindings` so we skip doctests entirely here.
-for DIR in "${WORKSPACE_MEMBERS[@]}"; do
-       RUSTFLAGS="$RUSTFLAGS --cfg=c_bindings" cargo test -p "$DIR" --verbose --color always --lib --bins --tests
-done
+RUSTFLAGS="$RUSTFLAGS --cfg=c_bindings" cargo test --verbose --color always --lib --bins --tests

 for DIR in lightning-invoice lightning-rapid-gossip-sync; do

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Just gonna land this. The changes are all straightforward moves/copies, and CI (finally) passes 🎉

@TheBlueMatt TheBlueMatt merged commit ad462bd into lightningdevkit:main Jan 14, 2025
25 checks passed
@TheBlueMatt TheBlueMatt mentioned this pull request Jan 14, 2025
@TheBlueMatt
Copy link
Collaborator

Backported in #3536.

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.

2 participants