-
Notifications
You must be signed in to change notification settings - Fork 113
fix(ci): enforce compatible CMake version for LND tests #516
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
Conversation
|
👋 I see @valentinewallace was un-assigned. |
c9480b5 to
e8dee26
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this! Two comments.
|
|
||
| - name: Check and install CMake if needed | ||
| # lnd_grpc_rust (via prost-build v0.10.4) requires CMake >= 3.5 but is incompatible with CMake >= 4.0. | ||
| # This step checks if CMake is missing, below 3.5, or 4.0 or higher, and installs CMake 3.31.5 if needed, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, 3.31.6 is the latest 3.x version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's the last version before 4.x. You can check all releases here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but you're downloading 3.31.5, not 3.31.6. Wondered if we wanted to use the latest version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I mistakenly added the wrong version, but I’ll update it to the latest one now.
| [ "$(cmake --version | head -n1 | cut -d' ' -f3)" \> "4.0" ]; then | ||
| sudo apt-get update | ||
| sudo apt-get remove -y cmake | ||
| wget https://github.com/Kitware/CMake/releases/download/v3.31.5/cmake-3.31.5-Linux-x86_64.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we verify the checksum to check we're downloading what we're expecting (similar to what we do in ./scripts/download_bitcoind_electrs.sh)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I’ve added checksum verification.
Ensure CMake is between 3.5 and 4.0 to prevent failures in LND integration tests caused by prost-build v0.10.4. This safeguards against version changes in ubuntu-latest. Closes lightningdevkit#515
e8dee26 to
d79d1ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
CI failures are unrelated.
Ensures CMake is between 3.5 and 4.0 in the CI workflow to prevent LND integration test failures caused by
prost-build v0.10.4. Since ubuntu-latest is not a fixed version, this safeguards against future runner updates that might introduce an incompatible CMake version.Closes #515.