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

pin v1.16 ahash to 0.8.3 #34650

Merged
merged 2 commits into from
Jan 5, 2024
Merged

pin v1.16 ahash to 0.8.3 #34650

merged 2 commits into from
Jan 5, 2024

Conversation

t-nelson
Copy link
Contributor

@t-nelson t-nelson commented Jan 4, 2024

Problem

poor upstream release management

Summary of Changes

pin ahash

@t-nelson
Copy link
Contributor Author

t-nelson commented Jan 4, 2024

@yihau is this not the command that's failing in CI ./cargo test --tests --verbose -p solana-cargo-build-sbf? it passes locally 🤔

@t-nelson
Copy link
Contributor Author

t-nelson commented Jan 4, 2024

ohhh i think it's because these test cargo.lock aren't under version control. the error comes from resolution of the index, to build the lockfile. mebe we can just commit them...

Comment on lines +1 to +2
# This file is automatically @generated by Cargo.
# It is not intended for manual editing.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

and yet, here i am!

@t-nelson t-nelson marked this pull request as ready for review January 4, 2024 21:55
@t-nelson
Copy link
Contributor Author

t-nelson commented Jan 4, 2024

@dmakarov on a scale of 1-10, how bad do you hate 3ad21cd ?

@dmakarov
Copy link
Contributor

dmakarov commented Jan 4, 2024

@dmakarov on a scale of 1-10, how bad do you hate 3ad21cd ?

  1. I don't mind this change at all. It shouldn't affect the tests it seems.

@t-nelson
Copy link
Contributor Author

t-nelson commented Jan 5, 2024

  1. I don't mind this change at all. It shouldn't affect the tests it seems.

great, thanks!

@willhickey
Copy link
Contributor

If I understand correctly this solves the problem without impacting the actual build. If that's correct I'd prefer this over 34645 to reduce risk for the v1.16 releases we need to bridge to v1.17

Copy link
Member

@yihau yihau left a comment

Choose a reason for hiding this comment

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

🔥

@t-nelson
Copy link
Contributor Author

t-nelson commented Jan 5, 2024

If I understand correctly this solves the problem without impacting the actual build. If that's correct I'd prefer this over 34645 to reduce risk for the v1.16 releases we need to bridge to v1.17

correct

@t-nelson t-nelson merged commit 471956c into solana-labs:v1.16 Jan 5, 2024
34 checks passed
@t-nelson t-nelson deleted the pa-1.16 branch January 5, 2024 03:14
yihau added a commit to yihau/solana that referenced this pull request Jan 9, 2024
@yihau
Copy link
Member

yihau commented Jan 9, 2024

bummer 🫠 I got this one.
Screenshot 2024-01-09 at 21 42 12

@t-nelson
Copy link
Contributor Author

t-nelson commented Jan 9, 2024

this is from secondary?

@CriesofCarrots
Copy link
Contributor

ahash v0.8.3 was yanked, so pinning to that specific version isn't going to work. v0.8.4 is probably the most-reasonable alternative, given how many old versions have been yanked.

@yihau
Copy link
Member

yihau commented Jan 9, 2024

this is from secondary?

yes

@yihau
Copy link
Member

yihau commented Jan 9, 2024

I'm thinking that maybe we don't need to pin the ahash version for all crates. we only pin the ahash version for programs. (keep sdk/cargo-build-sbf/tests/crates/fail/Cargo.lock and sdk/cargo-build-sbf/tests/crates/noop/Cargo.lock).

the original error originated from test_build. If users import our crates and not for a Solana program, they probably shouldn't be restricted by this 🤔

btw, I just checked the build log of v1.16.23. It seems that we have already compiled ahash 0.8.6 to crates. so I guess if we still prefer to pin ahash, 0.8.4 won't be so dangerous. (even 0.8.6)

@yihau
Copy link
Member

yihau commented Jan 10, 2024

I'm going to ping you in two different PRs for this issue 🪖

  1. v1.16: pin ahash to 0.8.4 #34723
  2. v1.16: unpin ahash for workspace #34716

(I prefer 1. atm. everything should be fixed magically and we don't need lock files for programs anymore. I guess it is also more friendly for users(?))

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.

5 participants