Fix `background-processor` doc build for `futures` #1744

tnull · 2022-09-26T12:43:43Z

This updates the docs so that they build with the futures feature enabled.

tnull · 2022-09-26T12:50:17Z

Added a CI check with 47f0101.

@TheBlueMatt does this sufficiently cover what you had in mind in #1739, or do we want further CI, e.g., running cargo test --features=futures?

codecov-commenter · 2022-09-26T13:04:53Z

Codecov Report

Base: 90.79% // Head: 90.78% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (0038a3f) compared to base (559ed20).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1744      +/-   ##
==========================================
- Coverage   90.79%   90.78%   -0.02%     
==========================================
  Files          87       87              
  Lines       46969    46969              
  Branches    46969    46969              
==========================================
- Hits        42645    42639       -6     
- Misses       4324     4330       +6

Impacted Files	Coverage Δ
lightning-background-processor/src/lib.rs	`96.23% <ø> (ø)`
lightning-block-sync/src/rest.rs	`62.06% <ø> (ø)`
lightning-block-sync/src/rpc.rs	`74.80% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`96.87% <0.00%> (-0.20%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.16% <0.00%> (+0.02%)`	⬆️
lightning/src/chain/channelmonitor.rs	`91.21% <0.00%> (+0.05%)`	⬆️
lightning/src/util/events.rs	`38.40% <0.00%> (+0.26%)`	⬆️
lightning/src/chain/onchaintx.rs	`95.63% <0.00%> (+0.91%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

.github/workflows/build.yml

TheBlueMatt · 2022-09-29T21:24:00Z

.github/workflows/build.yml

          - toolchain: 1.41.1
            build-no-std: false
            test-log-variants: true
+            build-futures: false


Why not test on MSRV? In fact, why not test on all builds?

Ah, I wasn't sure whether we wanted to give it that much resources. Now building on all platforms/toolchains with 1a55192.

Yikes, this just got a bit more relevant, since it fails to build under MSRV. To quote the futures repo README: "The current futures requires Rust 1.45 or later."

So 0.0.111 had an effective MSRV of 1.45 it seems ...

Now 'solved' by pinning the futures crates' versions to the last one with MSRV 1.41 (c367f71). Let me know if that's acceptable, or if we're okay with an MSRV of 1.45.2 for the lightning-background-processor crate.

I'm however unsure why macos-latest and windows-latest stable targets are hanging with "Waiting for status to be reported". Any ideas?

lightning-background-processor/src/lib.rs

.github/workflows/build.yml

TheBlueMatt · 2022-10-05T16:20:27Z

lightning-background-processor/Cargo.toml

+futures-executor = { version = "=0.3.17", optional = true }
+futures-io = { version = "=0.3.17", optional = true }
+futures-sink = { version = "=0.3.17", optional = true }
+futures-util = { version = "=0.3.17", optional = true }


Ah, we don't need to pin, and this is a good opportunity to drop most of the deps here - both here and in lightning-block-sync we can replace futures with futures-util = { version = "0.3", default-features = false }, which seems to build on 1.41 for me.

Hm, are you sure you didn't somehow still enable the default features locally (namely std and async-await-macro, which depends on futures-macros)? I can't reproduce that it works without, especially since select and Mutex seem to be only re-exported when they are set, e.g.:

https://github.com/rust-lang/futures-rs/blob/7a98cf0bbeb397dcfaf5f020b371ab9e836d33d4/futures-util/src/async_await/select_mod.rs#L315-L325

Also note that unpinning, i.e., version = "0.3" (abb55fd), leads to version 0.3.24 being used, which fails on 1.41.1. This was the reason I had to pin in the first place.

Ah, sorry, indeed, my test was bunk. A few things: (a) I think we should keep default-features = false, just use the async-await-macro feature, and swap to select_biased (which shouldn't matter for us, and its actually strictly less code - select just adds a random shuffle), (b) I think we should just bump the MSRV for lightning-background-processor/futures to 1.45.2, with block-sync and net-tokio.

Alright, now went this way with 19ae0b6.

Unfortunately, default-features = false doesn't work for lightning-block-sync though, since Mutex also requires std: https://github.com/rust-lang/futures-rs/blob/7a98cf0bbeb397dcfaf5f020b371ab9e836d33d4/futures-util/src/lock/mod.rs#L15-L18

tnull · 2022-10-07T10:39:59Z

Still unsure why two jobs are pending "status to be reported", as they seem to have finished just fine?

TheBlueMatt

LGTM, please squash. The "missing runs" thing I think is just cause the definition of the jobs changed so Github is sad. We'll have to temporarily disable requiring those jobs to merge but that's fine.

.github/workflows/build.yml

TheBlueMatt · 2022-10-09T16:42:34Z

Needs rebase.

tnull · 2022-10-10T14:58:17Z

Rebased and squashed changes addressing latest comments.

jkczyz · 2022-10-10T18:19:03Z

lightning-background-processor/src/lib.rs

-			select! {
+			select_biased! {


Would be good to have the reason for this change in the commit message or in a separate commit if unrelated.

Added more context in commit message with 0038a3f. Let me know if it is sufficient.

This adds testing of the `futures` feature to CI. In order to avoid introducing a dependency on `std`, we switch to using the `futures-util` crate directly enabling only a subset of features. To this end, we also switch to using the `select_biased!` macro, which is equivalent to `select!` except that it doesn't choose ready futures pseudo-randomly at runtime.

jkczyz reviewed Sep 26, 2022

View reviewed changes

.github/workflows/build.yml Show resolved Hide resolved

tnull force-pushed the 2022-09-fix-bp-futures-doc branch 3 times, most recently from 2a9b0dc to 07e7c25 Compare September 28, 2022 11:28

TheBlueMatt reviewed Sep 29, 2022

View reviewed changes

tnull force-pushed the 2022-09-fix-bp-futures-doc branch 5 times, most recently from 769e703 to c367f71 Compare September 30, 2022 10:52

TheBlueMatt reviewed Oct 5, 2022

View reviewed changes

tnull force-pushed the 2022-09-fix-bp-futures-doc branch 12 times, most recently from 1e792a9 to abb55fd Compare October 6, 2022 11:43

TheBlueMatt added this to the 0.0.112 milestone Oct 7, 2022

tnull force-pushed the 2022-09-fix-bp-futures-doc branch 2 times, most recently from 0880c89 to 19ae0b6 Compare October 7, 2022 09:22

TheBlueMatt reviewed Oct 7, 2022

View reviewed changes

.github/workflows/build.yml Show resolved Hide resolved

.github/workflows/build.yml Outdated Show resolved Hide resolved

Fix background-processor doc build for futures

d3c369d

tnull force-pushed the 2022-09-fix-bp-futures-doc branch 2 times, most recently from 9957d3a to 8e93bd4 Compare October 10, 2022 14:57

TheBlueMatt previously approved these changes Oct 10, 2022

View reviewed changes

jkczyz reviewed Oct 10, 2022

View reviewed changes

tnull dismissed TheBlueMatt’s stale review via 0038a3f October 10, 2022 18:54

tnull force-pushed the 2022-09-fix-bp-futures-doc branch from 8e93bd4 to 0038a3f Compare October 10, 2022 18:54

jkczyz approved these changes Oct 10, 2022

View reviewed changes

tnull requested a review from TheBlueMatt October 10, 2022 19:16

TheBlueMatt approved these changes Oct 10, 2022

View reviewed changes

TheBlueMatt merged commit 6738fd5 into lightningdevkit:main Oct 10, 2022

Fix background-processor doc build for futures #1744

Fix background-processor doc build for futures #1744

Uh oh!

Conversation

tnull commented Sep 26, 2022

Uh oh!

tnull commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

TheBlueMatt Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

tnull Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

tnull Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Oct 6, 2022

Choose a reason for hiding this comment

Uh oh!

tnull Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Oct 7, 2022

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Oct 9, 2022

Uh oh!

tnull commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkczyz Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

tnull Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fix `background-processor` doc build for `futures` #1744

Fix `background-processor` doc build for `futures` #1744

tnull commented Sep 26, 2022 •

edited

Loading

codecov-commenter commented Sep 26, 2022 •

edited

Loading

tnull Sep 30, 2022 •

edited

Loading

tnull Sep 30, 2022 •

edited

Loading

tnull Sep 30, 2022 •

edited

Loading

tnull Oct 6, 2022 •

edited

Loading

tnull Oct 6, 2022 •

edited

Loading

tnull Oct 7, 2022 •

edited

Loading

tnull commented Oct 10, 2022 •

edited

Loading

tnull Oct 10, 2022 •

edited

Loading