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

Check for changes to the public API #34

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

tcharding
Copy link
Member

We would like to get to a stage where we can commit to the public API. To help us achieve this add a script that generates the public API and checks it against three committed files, one for each feature set: no features, alloc, std.

The idea is that with this applied any PR that changes the public API should include a final patch that is just the changes to the api/*.txt files, that way reviewers can discuss the changes without even needing to look at the code, quickly giving concept ACK/NACKs. We also run the script in CI to make sure we have not accidentally changed the public API so that we can be confident that don't break semver during releases. The script can also be used to diff between two release versions to get a complete list of API changes, useful for writing release notes and for users upgrading.

There is a development burden involved if we apply this patch.

@tcharding
Copy link
Member Author

This includes one for "core2" and one for "alloc", separately because its easy to mess up feature gates with these two.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK cbfecd0

@tcharding
Copy link
Member Author

@Kixunil are you good with this?

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Concept ACK.

$CMD --no-default-features | sort --unique > "$API_DIR/no-default-features.txt"
$CMD | sort --unique > "$API_DIR/default-features.txt"
$CMD --no-default-features --features=alloc | sort --unique > "$API_DIR/alloc.txt"
$CMD --no-default-features --features=core2 | sort --unique > "$API_DIR/core2.txt"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you try sorting with -n? It would read better if the array types were sorted naturally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Using -n (or -h) causes the script to produce no output.

Copy link
Collaborator

Choose a reason for hiding this comment

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

WTF, that's some weird interaction between -n and --unique. You can fix it by writing sort -n | uniq

Copy link
Member Author

@tcharding tcharding Nov 30, 2023

Choose a reason for hiding this comment

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

So I think we have pushed bash as far as it can reasonably go, bash can't easily sort the following 4 lines as we want AFAIK

impl<'a> hex_conservative::display::DisplayHex for &'a [u8; 9]
impl<'a> hex_conservative::display::DisplayHex for &'a [u8; 8]
pub type [u8; 128]::Err = hex_conservative::HexToArrayError
pub type [u8; 10]::Err = hex_conservative::HexToArrayError

Are you guys ok with a perl script to get the order to be:

impl<'a> hex_conservative::display::DisplayHex for &'a [u8; 8]
impl<'a> hex_conservative::display::DisplayHex for &'a [u8; 9]
pub type [u8; 10]::Err = hex_conservative::HexToArrayError
pub type [u8; 128]::Err = hex_conservative::HexToArrayError

Is there a better tool than perl for this? (FTR neither my perl nor awk are good enough to quickly write this but since we are going to use this script everywhere I can allocate some time to it.)

Copy link
Member

Choose a reason for hiding this comment

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

Either awk or perl is fine by me. I can read, but not really write, those languages.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a fan of perl but if it's the simplest choice for you I could tolerate it.

@tcharding tcharding force-pushed the 10-26-check-public-api branch from cbfecd0 to 5d7eaf5 Compare November 20, 2023 03:44
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 5d7eaf5

We would like to get to a stage where we can commit to the public API.
To help us achieve this add a script that generates the public API and
checks it against three committed files, one for each feature set: no
features, alloc, std.

The idea is that with this applied any PR that changes the public API
should include a final patch that is just the changes to the api/*.txt
files, that way reviewers can discuss the changes without even needing
to look at the code, quickly giving concept ACK/NACKs. We also run the
script in CI to make sure we have not accidentally changed the public
API so that we can be confident that don't break semver during releases.
The script can also be used to diff between two release versions to get
a complete list of API changes, useful for writing release notes and for
users upgrading.

There is a development burden involved if we apply this patch.
@tcharding tcharding force-pushed the 10-26-check-public-api branch from 5d7eaf5 to b45bd6e Compare November 30, 2023 20:59
@tcharding
Copy link
Member Author

Added piping through uniq which is clearly better. Whether or not we should improve the sort order I don't know, I can definitely put an hour or so into it.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK b45bd6e

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK b45bd6e

@apoelstra apoelstra merged commit 7866965 into rust-bitcoin:master Dec 4, 2023
@tcharding tcharding deleted the 10-26-check-public-api branch December 6, 2023 03:27
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