Check for changes to the public API by tcharding · Pull Request #2713 · rust-bitcoin/rust-bitcoin

tcharding · 2024-04-24T22:44:46Z

This PR is #1880 re-opened.

Add a script that checks the public API of hashes and bitcoin. Document how to use it during development. Call it in CI. Do not add it to githooks because the githooks because its expected to be run per PR not per commit.

Includes a just command to run the script: just check-api

Implied workflow change

This PR imposes workflow changes.

Explicitly: all PRs that change the public API of bitcoin, base58, hashes, io, or units must contain changes to the api text files.

Suggestion: We add the patch updating api text files as a separate patch at the end of each PR so we can haggle over it separately from the actual code changes.

Fix: #1875

coveralls · 2024-04-24T22:46:47Z

Pull Request Test Coverage Report for Build 8863062354

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 83.131%

Totals
Change from base Build 8849264030:	0.0%
Covered Lines:	19195
Relevant Lines:	23090

💛 - Coveralls

apoelstra · 2024-04-25T12:04:01Z

contrib/check-for-api-changes.sh

In eb1f4030afc0f40b7ba8df308fbfd1c11521d1c6:

trailing whitespace

It is still pending this @tcharding.

My bad, thanks. Fixed now.

apoelstra · 2024-04-25T12:09:04Z

Note that even though we agreed to just push forward on this, it may take a bit to get in because (a) we still need two ACKs, unless it sits open for 2 weeks without any comments, and (b) it will need to be rebased every time we merge something that changes the API.

apoelstra

ACK d9656b9630acf0ec99057b520a94e99470e83b59

tcharding · 2024-04-25T21:32:52Z

After two months of being kind of stressed (as stressed as a bloke who works at home in tracksuit pants can be anyways) I found myself this last couple of days in a strange place of calm, just accepting that likely nothing is going to merge any time soon. Finally making some headway in hashes had me actually excited to start work this morning, first time in what seems like ages.

I kind of think we should do your idea of branching off and doing the hashes re-write then merging later when we have it nailed down. Although a bunch of my work is still so exploratory that I'm not sure which stuff is going to work. This week I already deleted a bunch of branches and started from scratch again.

storopoli · 2024-04-26T09:44:27Z

api/README.md

I prefer that documentation is consistent.
All .md files in the repo uses the GH flavor, e.g.

rust-bitcoin/CONTRIBUTING.md

Lines 1 to 5 in a9cdcb4

# Contributing to rust-bitcoin

:+1::tada: First off, thanks for taking the time to contribute! :tada::+1:

The following is a set of guidelines for contributing to Rust Bitcoin

Suggested change

API text files

==============

# API text files

Ah yeah, sure thing. Thanks!

I tend to use ==== in my own stuff so I went to check that I hadn't introduced any other files and there are tons in this repo

gg '====' base58/README.md:2: ======================= bitcoin/tests/data/README.md:2: ================ bitcoin/tests/data/serde/README.md:2: ========================== fuzz/README.md:103:===================================================================== internals/README.md:2: ====================== io/README.md:2: ======================= units/README.md:2: =============

We should probably make them all uniform.

gg is an alias to git grep -IPn --color=always

Yes my

All .md files

was based on a sample from the root dir .mds. I should have been more specific :/

We should definitely normalize all of them.
I can work in a future PR to normalize markdown, I am quite used from an old gig that I was in charge of technical documentation in scientific software.

Sweet, nice one.

storopoli · 2024-04-26T09:46:00Z

api/README.md

Also this can be a bash fenced block no?

Done, thanks.

tcharding · 2024-04-26T22:45:06Z

Force push is the two suggested fixes.

apoelstra

ACK f228f54ff5f5e1c771617a5cd4247110eba66f23

storopoli

ACK 7e5ff49702221c64e4960b72be979541b168a965

tcharding · 2024-05-17T23:52:14Z

Re-based on master, re-ran the script, updated the github action to use new style.

We would like to check for API changes during development and in CI so that such changes can be discussed separately from the code. Add tooling and documentation for creating API listings for the public crates within the workspace (i.e., not `internals`). Add API text files from an initial run of the script.

Add a job that runs the new script to check for changes to the public APIs of `hashes` and `bitcoin`.

Add a `just` command to run the API checking script. Makes it more discoverable.

tcharding · 2024-05-17T23:55:40Z

shellcheck for the win! I had a failing job that found a bug because I'd forgotten to shellcheck the script, well done @storopoli for introducing the CI shellcheck job.

storopoli · 2024-05-18T08:57:17Z

shellcheck for the win! I had a failing job that found a bug because I'd forgotten to shellcheck the script, well done @storopoli for introducing the CI shellcheck job.

Nice I was expecting it to payoff but not that fast ROI...
This probably bumps up the need to add shellcheck to other rust-bitcoin crates.
I'll see what I can cook up this weekend.

apoelstra

ACK 76331ae normally would complain about the whitespace but I would like to ACK/merge this quickly since most other PR that gets merged will force it to be rebased. also will one-ACK merge as "no NACKs or comments from maintainers in 2 weeks"

tcharding · 2024-05-18T22:52:40Z

I went looking for the whitespace issue but couldn't find it. Did #2780 though.

apoelstra · 2024-05-19T15:45:00Z

@tcharding you introduced trailing whitespace in your first commit and removed it in the last one.

tcharding · 2024-05-19T21:23:28Z

Ah, woops. Cheers.

github-actions bot added ci doc labels Apr 24, 2024

tcharding force-pushed the 05-26-check-public-api branch from 42dad8b to d9656b9 Compare April 24, 2024 22:59

github-actions bot added the test label Apr 24, 2024

apoelstra reviewed Apr 25, 2024

View reviewed changes

apoelstra previously approved these changes Apr 25, 2024

View reviewed changes

storopoli reviewed Apr 26, 2024

View reviewed changes

tcharding dismissed apoelstra’s stale review via 1aaf666 April 26, 2024 22:43

tcharding force-pushed the 05-26-check-public-api branch 2 times, most recently from 1aaf666 to f228f54 Compare April 26, 2024 22:44

apoelstra previously approved these changes Apr 27, 2024

View reviewed changes

tcharding dismissed apoelstra’s stale review via 7e5ff49 April 27, 2024 22:40

tcharding force-pushed the 05-26-check-public-api branch from f228f54 to 7e5ff49 Compare April 27, 2024 22:40

storopoli approved these changes Apr 27, 2024

View reviewed changes

tcharding force-pushed the 05-26-check-public-api branch from 7e5ff49 to 32e67a1 Compare May 17, 2024 23:51

github-actions bot removed the test label May 17, 2024

tcharding added 3 commits May 18, 2024 09:54

CI: Add job to check for API changes

b222f40

Add a job that runs the new script to check for changes to the public APIs of `hashes` and `bitcoin`.

Add a just cmd to check for API changes

76331ae

Add a `just` command to run the API checking script. Makes it more discoverable.

tcharding force-pushed the 05-26-check-public-api branch from 32e67a1 to 76331ae Compare May 17, 2024 23:55

apoelstra mentioned this pull request May 18, 2024

p2p: Cleanup test imports #2775

Merged

apoelstra approved these changes May 18, 2024

View reviewed changes

apoelstra merged commit 13569b8 into rust-bitcoin:master May 18, 2024

tcharding deleted the 05-26-check-public-api branch May 23, 2024 03:39

Kixunil mentioned this pull request Jul 1, 2024

Check for API breaking changes in CI #1875

Closed

tcharding mentioned this pull request Dec 4, 2024

Add API scripts and output files #3682

Merged

	# Contributing to rust-bitcoin

	:+1::tada: First off, thanks for taking the time to contribute! :tada::+1:

	The following is a set of guidelines for contributing to Rust Bitcoin

Conversation

tcharding commented Apr 24, 2024

Implied workflow change

Uh oh!

coveralls commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 8863062354

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Apr 25, 2024

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Apr 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Apr 26, 2024

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

storopoli left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented May 17, 2024

Uh oh!

tcharding commented May 17, 2024

Uh oh!

storopoli commented May 18, 2024

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented May 18, 2024

Uh oh!

apoelstra commented May 19, 2024

Uh oh!

tcharding commented May 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Apr 24, 2024 •

edited

Loading