Skip to content

Conversation

@yihau
Copy link
Member

@yihau yihau commented Nov 12, 2025

Problem

I noticed that some crates' frozen-abi features can't be run individually. The reason is that our tests run them all together, which allows them to rely on tainted packages and hide potential errors. As you can see in https://github.com/anza-xyz/solana-sdk/actions/runs/19302341784/job/55200264398, it caught a missing solana_short_vec/frozen-abi in the message.

Summary of Changes

test frozen-abi crate by crate

@yihau yihau requested a review from joncinque November 12, 2025 15:35
@yihau yihau marked this pull request as ready for review November 12, 2025 15:35
@yihau yihau requested a review from a team as a code owner November 12, 2025 15:35
tao-stones
tao-stones previously approved these changes Nov 12, 2025
Copy link
Contributor

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

looks good for metadata

Copy link
Collaborator

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! Just a small suggestion that might simplify the change

Comment on lines 8 to 13
packages=$(./cargo nightly metadata --no-deps --format-version=1 | jq -r '.packages[] | select(.features | has("frozen-abi")) | .name')
for package in $packages; do
echo "::group::./cargo nightly test -p $package --features frozen-abi --lib -- test_abi_ --nocapture"
./cargo nightly test -p "$package" --features frozen-abi --lib -- test_abi_ --nocapture
echo "::endgroup::"
done
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to use cargo hack, we can simplify the change to simply:

./cargo nightly hack --features frozen-abi --ignore-unknown-features test --lib -- test_abi_ --nocapture

What do you think? We'll also need to install cargo-hack for this step in CI if we go this route

Copy link
Member Author

Choose a reason for hiding this comment

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

sure! let's do it: c086900

Copy link
Collaborator

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@yihau yihau requested a review from tao-stones November 13, 2025 14:36
@yihau yihau merged commit c667d88 into anza-xyz:master Nov 13, 2025
24 checks passed
@yihau yihau deleted the fix-frozen-abi branch November 13, 2025 14:50
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.

3 participants