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

Remove the lightning-transaction-sync MSRV #2055

Merged
merged 3 commits into from
Feb 27, 2023

Conversation

TheBlueMatt
Copy link
Collaborator

Apparently it has dependencies that don't track an MSRV at all, so
we can't practically enforce one in CI.

Apparently it has dependencies that don't track an MSRV at all, so
we can't practically enforce one in CI.
@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2023

Codecov Report

Base: 87.26% // Head: 87.25% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (64a7567) compared to base (16b3c72).
Patch coverage: 73.88% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2055      +/-   ##
==========================================
- Coverage   87.26%   87.25%   -0.02%     
==========================================
  Files         101      100       -1     
  Lines       44414    44480      +66     
  Branches    44414    44480      +66     
==========================================
+ Hits        38759    38810      +51     
- Misses       5655     5670      +15     
Impacted Files Coverage Δ
lightning/src/ln/channelmanager.rs 86.54% <0.00%> (+<0.01%) ⬆️
lightning/src/ln/onion_utils.rs 86.70% <ø> (ø)
lightning/src/routing/gossip.rs 83.83% <ø> (-0.16%) ⬇️
lightning/src/util/errors.rs 32.35% <0.00%> (+0.21%) ⬆️
lightning/src/util/events.rs 25.50% <28.57%> (-0.43%) ⬇️
lightning/src/util/ser.rs 84.27% <33.33%> (-0.46%) ⬇️
lightning-background-processor/src/lib.rs 81.88% <75.00%> (+0.20%) ⬆️
lightning/src/util/ser_macros.rs 91.59% <75.00%> (-1.10%) ⬇️
lightning/src/ln/outbound_payment.rs 80.23% <90.74%> (+1.17%) ⬆️
lightning/src/chain/channelmonitor.rs 89.63% <100.00%> (+0.04%) ⬆️
... and 14 more

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.

jkczyz
jkczyz previously approved these changes Feb 27, 2023
.github/workflows/build.yml Outdated Show resolved Hide resolved
@@ -144,7 +144,7 @@ jobs:
run: |
cd lightning && cargo test --verbose --color always --features backtrace
- name: Test on Rust ${{ matrix.toolchain }} with net-tokio
if: "matrix.build-net-tokio && !matrix.coverage && matrix.build-tx-sync"
if: "matrix.build-net-tokio && !matrix.coverage"
Copy link
Contributor

Choose a reason for hiding this comment

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

In line 137, can we assume that if matrix.coverage is not set then it is false? If so, we already have a build covering that in line 125.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure what you're asking? We have two builds, one for coverage and one for not-coverage, the coverage one sets link-dead-code.

Copy link
Contributor

@wpaulino wpaulino Feb 27, 2023

Choose a reason for hiding this comment

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

Right, and on line 136, there's another build for lightning-transaction-sync where the condition doesn't check for matrix.coverage at all. Just wondering if there's a difference here, because if there isn't, then that build could be removed.

Because `lightning-transaction-sync` does not have an MSRV (and
because its dev-dependencies are huge), we can't build it by
default when devs run `cargo test`, so it is moved out of the
top-level workspace.
@TheBlueMatt
Copy link
Collaborator Author

Fixed the bonus transaction-sync test run:

$ git diff-tree -U1 64a75679 c4f2e487
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 9fb37f1b4..cebaa9a46 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -135,8 +135,2 @@ jobs:
           RUSTFLAGS="-C link-dead-code" cargo build --verbose --color always --features esplora-async
-      - name: Test transaction sync clients on Rust ${{ matrix.toolchain }} with features
-        if: "matrix.build-tx-sync"
-        run: |
-          cd lightning-transaction-sync
-          cargo test --verbose --color always --features esplora-blocking
-          cargo test --verbose --color always --features esplora-async
       - name: Test backtrace-debug builds on Rust ${{ matrix.toolchain }}

@tnull
Copy link
Contributor

tnull commented Feb 27, 2023

Fixed the bonus transaction-sync test run:

$ git diff-tree -U1 64a75679 c4f2e487
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 9fb37f1b4..cebaa9a46 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -135,8 +135,2 @@ jobs:
           RUSTFLAGS="-C link-dead-code" cargo build --verbose --color always --features esplora-async
-      - name: Test transaction sync clients on Rust ${{ matrix.toolchain }} with features
-        if: "matrix.build-tx-sync"
-        run: |
-          cd lightning-transaction-sync
-          cargo test --verbose --color always --features esplora-blocking
-          cargo test --verbose --color always --features esplora-async
       - name: Test backtrace-debug builds on Rust ${{ matrix.toolchain }}

I'm confused: why are these 'bonus' again? We now only have cases where the tx-sync crate is built, but none that run the tests?

@TheBlueMatt
Copy link
Collaborator Author

Grrrr, apparently @wpaulino and I both can't read - I missed that it was separate between test/build.

Cargo.toml Show resolved Hide resolved
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.

None yet

5 participants