Skip to content

Conversation

@TheBlueMatt
Copy link
Collaborator

While we should keep testing MacOS and Windows builds (especially Winblowz since its somehow still cooperative multitasking within a process in 2025), there's not a lot of need to test rustc beta builds on them, which we mostly do just to ensure we aren't going to be broken by a future rustc update that breaks some API (which is exceedingly rare).

This should get our CI queueing times back close to zero on all jobs.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jul 24, 2025

I've assigned @tankyleo as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@TheBlueMatt TheBlueMatt force-pushed the 2025-07-less-winblowz branch from 733e889 to 9f55cc5 Compare July 24, 2025 19:25
While we should keep testing MacOS and Windows builds (especially
Winblowz since its somehow still cooperative multitasking within a
process in 2025), there's not a lot of need to test rustc beta
builds on them, which we mostly do just to ensure we aren't going
to be broken by a future rustc update that breaks some API (which
is exceedingly rare).

This should get our CI queueing times back close to zero on all
jobs.
@TheBlueMatt TheBlueMatt force-pushed the 2025-07-less-winblowz branch from 9f55cc5 to 2bbea3b Compare July 24, 2025 19:26
@ldk-reviews-bot ldk-reviews-bot requested a review from tankyleo July 24, 2025 19:34
@codecov
Copy link

codecov bot commented Jul 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.92%. Comparing base (492fa70) to head (035ed04).
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3958      +/-   ##
==========================================
+ Coverage   88.72%   88.92%   +0.19%     
==========================================
  Files         172      173       +1     
  Lines      123553   123794     +241     
  Branches   123553   123794     +241     
==========================================
+ Hits       109625   110084     +459     
+ Misses      11552    11249     -303     
- Partials     2376     2461      +85     
Flag Coverage Δ
fuzzing 22.21% <ø> (?)
tests 88.75% <ø> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tankyleo
tankyleo previously approved these changes Jul 24, 2025
@ldk-reviews-bot
Copy link

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

toolchain: [ stable, beta, 1.63.0 ] # 1.63.0 is the MSRV for all crates but `lightning-transaction-sync`.
exclude:
- platform: windows-latest
toolchain: 1.63.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not also exclude 1.63 on macOS?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should still support the MSRV if we have any macOS-dependent code. The only reason its skipped on Windows is we don't support an MSRV on Windows because of some dependency crap (but maybe we could now that we build tx-sync separately?).

I pushed an update to test if WIndows builds with 1.63 again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lol, nevermind 🤷‍♂️

error: "D:\a\rust-lightning\rust-lightning\target\doc\windows_sys\all.html": The system cannot find the path specified. (os error 3)

error: couldn't generate documentation: I/O error

error: could not document `windows-sys`

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, in this case lightning-transaction-sync wasn't the culprit for once.

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

IIRC, sometime in the past there was an issue that was only appearing on macOS and Rust beta channel (can't fully recall though). I guess we can/should still do this, but want to note that we of course reduce coverage a bit, but that's probably okay.

Landing this as it's simple enough.

@tnull tnull merged commit 39e8d7d into lightningdevkit:main Jul 27, 2025
126 of 174 checks passed
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.

6 participants