Check for changes to the public API #2713

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

+    popd > /dev/null
+}
+
+# Check if there are changes (dirty git index) to the `api/` directory. 


In eb1f403:

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 d9656b9

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

+API text files
+==============


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

+Requires `cargo-public-api`, install with:
+
+    `cargo +stable install cargo-public-api --locked`.


Also this can be a bash fenced block no?

Done, thanks.

We have the `$cargo_ver` variable already, use it in the error message instead of the duplicate call to `$(cargo --version)`.

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`.

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

Force push is the two suggested fixes.

apoelstra

ACK f228f54

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

storopoli

ACK 7e5ff49

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

CI: Use local variable instead of duplicate command

c317a7b

We have the `$cargo_ver` variable already, use it in the error message instead of the duplicate call to `$(cargo --version)`.

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

tcharding force-pushed the 05-26-check-public-api branch from d9656b9 to 1aaf666 Compare April 26, 2024 22:43

tcharding added 2 commits April 27, 2024 08:44

CI: Add job to check for API changes

b6b4dde

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

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

apoelstra previously approved these changes Apr 27, 2024

View reviewed changes

Add a just cmd to check for API changes

7e5ff49

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for changes to the public API #2713

Check for changes to the public API #2713

tcharding commented Apr 24, 2024

coveralls commented Apr 24, 2024 •

edited

apoelstra Apr 25, 2024

storopoli Apr 27, 2024

tcharding Apr 27, 2024

apoelstra commented Apr 25, 2024

apoelstra left a comment

tcharding commented Apr 25, 2024

storopoli Apr 26, 2024

tcharding Apr 26, 2024

tcharding Apr 26, 2024

tcharding Apr 26, 2024

storopoli Apr 26, 2024

tcharding Apr 27, 2024

storopoli Apr 26, 2024

tcharding Apr 26, 2024

tcharding commented Apr 26, 2024

apoelstra left a comment

storopoli left a comment

	# 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

		Requires `cargo-public-api`, install with:

		`cargo +stable install cargo-public-api --locked`.

Check for changes to the public API #2713

Are you sure you want to change the base?

Check for changes to the public API #2713

Conversation

tcharding commented Apr 24, 2024

Implied workflow change

coveralls commented Apr 24, 2024 • edited

Pull Request Test Coverage Report for Build 8863062354

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Apr 25, 2024

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Apr 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Apr 26, 2024

apoelstra left a comment

Choose a reason for hiding this comment

storopoli left a comment

Choose a reason for hiding this comment

coveralls commented Apr 24, 2024 •

edited