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

chore: pin crunchy for Windows compatibility #1670

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

carver
Copy link
Collaborator

@carver carver commented Feb 6, 2025

What was wrong?

Windows build is failing: https://github.com/ethereum/trin/actions/runs/13189513029/job/36819458339

How was it fixed?

Pin crunchy for all dependencies

@carver carver requested a review from KolbyML February 6, 2025 23:15
@carver
Copy link
Collaborator Author

carver commented Feb 6, 2025

Still working on testing it locally

@KolbyML
Copy link
Member

KolbyML commented Feb 6, 2025

Still working on testing it locally

Shouldn't you push the updated lock file as well?

@carver
Copy link
Collaborator Author

carver commented Feb 6, 2025

Yeah, I will before taking it out of Draft. But I'm finding that it locally is still allowing crunchy to go to 0.2.3

@KolbyML
Copy link
Member

KolbyML commented Feb 6, 2025

Yeah, I will before taking it out of Draft. But I'm finding that it locally is still allowing crunchy to go to 0.2.3

rev must be used instead of ref to get the desired outcome

Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

:shipit: PR overall looks good, of course pushing the updated lock files which uses the patch is required for CI to pass

Cargo.toml Outdated
# get the wrong path, update this when the workflow has been updated
#
# See: https://github.com/eira-fransham/crunchy/issues/13
crunchy = { git = "https://github.com/eira-fransham/crunchy", ref = "1bf90cf2d0a8cfcb2c5592275a23ab028dff6468" }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
crunchy = { git = "https://github.com/eira-fransham/crunchy", ref = "1bf90cf2d0a8cfcb2c5592275a23ab028dff6468" }
crunchy = { git = "https://github.com/eira-fransham/crunchy", rev = "1bf90cf2d0a8cfcb2c5592275a23ab028dff6468" }

this must be rev instead of ref

@carver
Copy link
Collaborator Author

carver commented Feb 6, 2025

Weird that the tooling didn't complain about ref. It's maybe the only time that Rust tooling has been too relaxed for my taste. X)

@carver carver marked this pull request as ready for review February 6, 2025 23:36
Need the patch for all the sub-dependencies
@carver carver merged commit a92ea33 into ethereum:master Feb 7, 2025
11 checks passed
@carver carver deleted the pin-crunchy branch February 7, 2025 00:06
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