Add a script to generate API files using `cargo public-api` for the
crates that we are trying to stabalise (the so called 'leaf crates').
- hashes
- io
- primitives
- units
We already ran the script and committed the files last patch. The fact
that this patch does not include any changes to the `api/` directory and
that CI passes is enough to convince us that last patch was valid.
Add a CI job that runs the script and checks there are no changes to
the committed text files thereby proving no API changes are made in a
PR without explicitly modifying the API text files.
Add documentation to `CONTRIBUTING.md` on what is expected of devs.
Example code in rustdocs should be error free and not produce warnings
when copy pasted.
The section on rustdocs has been expanded to clarify that example code
should compile without any errors or warnings with the stricter `lint`
that has been added to the justfile.
Fix list items to use capital letters because the list items are
sentences (have trailing full stop already).
Also, use a long single line because it is [subjectively] easier to read
the list.
As much as it hurts the C hacker inside me we have settled on using
`_internal` to mark private function names that clash with a public
function of the same name.
Introduce a policy section and rename one instance, I did not grep the
codebase looking for other violations.
This came up because I had to look at what `_inner` implied when reading
the function name somewhere else.
- adds an item that all error messages
should be lower case, except for proper nouns and variable names.
- adds a section on expect messages,
following discussion in #3019 and #3053.
Done to try to further help new contributors and save maintainer time.
Add an example of using `just check-api` and an example `git` command
for devs to create the api patch now required for all changes to the
public API.
f3d364ef1d reduce two-ACK requirement to one-ACK requirement (Andrew Poelstra)
Pull request description:
For the last few months it has only been myself and tcharding working on this project actively. Prior to that we had Kixunil as well, which allowed us to move forward requiring two ACKs on every PR (though it put a minimum 24-hour delay on everything since we are pairwise 8 hours separated from each other in timezones).
The other listed maintainers, especially Sanket and Matt, are intermittently available, which is awesome but insufficient to get large refactors or long-term work through.
We have increasingly depended on the "one-ACK carveout", an ever-increasing list of exceptions to our two-ACK rule. Most silly, one exception is that if something stays open for two weeks, we don't need the second ACK. So the result is a "one ACK plus 2 weeks" policy which is grating and demoralizing.
Meanwhile, we are not finished our crate-smashing project or our API overhaul, and we want to continue moving at a fast rate. (Though of course, we will take breakage seriously; we do not want to make gratuitous changes or changes that replace previous changes, and the changes we *do* make we will do our damnedest to do via a year-long deprecation cycle. And we welcome suggestions to improve our policy or process on this front. But requiring extra maintainers/reviewers has not been helpful for us.)
I'll let this PR sit open for 7 days (til Wed May 22) to accumulate ACKs or NACKs, and assuming no strong opposition, will merge it.
ACKs for top commit:
sanket1729:
ACK f3d364ef1d
Tree-SHA512: 6d1ea41ce79b038304c59d574b239fc855cc178dc61b4dd8e20dc9e0493278df339950a7604592dd20d7b25c8c56f6d4b5c13b103771a8b920ec10d4050578ae
98d6ab3865 policy: Mention format of error variants (Tobin C. Harding)
Pull request description:
An error enum type should not use an `Error` prefix for its variants, mention as such in the policy docs.
ACKs for top commit:
apoelstra:
ACK 98d6ab3865
Tree-SHA512: 16375ff031af24e7b1a5e8206c9660512b26da86df4168ac4a18e48f118c455460d0fee78e1e1ce972a32daba9aff7fcc9d0043bfd21bae223f95d9e944330ca
7de02da9f5 policy: Introduce error type re-export policy (Tobin C. Harding)
Pull request description:
We have started introducing `error` sub-modules for modules that have a lot of error code. This is mainly a code organisation thing.
When importing a function it is annoying to have to go to another module to get the returned error type. Furthermore it leaks our module structure and introduces a maintenance burden because for modules that do not [yet] have an `error` sub-module the error types are in a different place to modules that do have an `error` sub-module.
Furthermore, if function users have to go to the `error` sub-module to read docs etc. they are bombarded with all the hider errors as well (see definitions below).
We can solve both problems by re-exporting all regular errors, making the `error` sub-module public (for niche users), and not re-exporting hider errors.
I believe the re-export should only be done in the module that holds `error` sub-module i.e., do not re-export `foo::error::SomeError` from `bar` module even if `bar::some_function` returns `SomeError`.
Definitions:
- hider error: An error type, often a struct, that is nested in another error just to hide the internals.
- regular error: An error type that is returned from a public function, most often an enum.
Please note that some times errors act as both hider errors and regular errors so the definitions are not perfect.
ACKs for top commit:
apoelstra:
ACK 7de02da9f5 LGTM
Tree-SHA512: 42b9d9b78c882d39553c3d72c188d80f5e305e1f7f1b9b9212760c75ff35ea71e868c93a17851ca0bf2104b01d5c6d316cd14c422c262ff39ad72922c562af4a
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 section on how we do backports and what the merge policy is.
Recently I backported a patch by pushing directly to the `0.32.x`
release branch but this is not an optimal process because it leaves no
history on github. Instead PR backports in like we do for patching
`master` but allow one-ACK merging if the backport is a straight cherry
pick of a patch from `master`.
9b70c65f5d Introduce new one-ack carve out rule (Tobin C. Harding)
42d02fbd66 Merge Refactor and One ACK carve outs (Tobin C. Harding)
ebf5b670d4 Update test script mention (Tobin C. Harding)
Pull request description:
Update merge carve-out policy and introduce new rule.
- Patch 1: Fix stale test script mention
- Patch 2: Merge current carve-outs into a single carve-out with multiple rules
- Patch 3: Introduce new carve-out rule
From patch 3:
```
Introduce new one-ack carve out rule
Our merge process is being artificially slowed down because of a
combination of:
- Using merge-commit merging means PRs often have to be rebased with no
changes but a different merge base (and force pushed).
- Trivial changes, like fixing nits, are often force pushed also.
- Force pushes invalidate ACKs
- Our devs are spread around the world working at different times
What this means is trivial force pushes often cause multi day delays in
merging. To try and alleviate this problem introduce an additional rule
to the One ACK carve-out so that Andrew can merge PRs that have
previously been ack'ed by another dev and have only minimal changes.
The definition of "trivial" is subjective which introduces a burden on
Andrew to not merge stuff willy-nilly but also allows simple changes to
the original PR (eg fixed nits that the original reviewer suggested).
```
ACKs for top commit:
apoelstra:
ACK 9b70c65f5d confirmed via range-diff that the commit everyone ACKed and this one differ only in `as` vs `has`
Tree-SHA512: 41898e71e013ac70e41bb4624ce5e5055dc3e7a405dd73d3988f5b02ece104d7fad746203ce8d26a6a33f98b745010fc39e9a4bddb9bcf22267c942a4dac2028
Our merge process is being artificially slowed down because of a
combination of:
- Using merge-commit merging means PRs often have to be rebased with no
changes but a different merge base (and force pushed).
- Trivial changes, like fixing nits, are often force pushed also.
- Force pushes invalidate ACKs
- Our devs are spread around the world working at different times
What this means is trivial force pushes often cause multi day delays in
merging. To try and alleviate this problem introduce an additional rule
to the One ACK carve-out so that Andrew can merge PRs that have
previously been ack'ed by another dev and have only minimal changes.
The definition of "trivial" is subjective which introduces a burden on
Andrew to not merge stuff willy-nilly but also allows simple changes to
the original PR (eg fixed nits that the original reviewer suggested).
The "One ACK carve-out" has 3 rules and then there is a separate
"Refactor carve-out" that covers things that are not only refactoring -
this makes it hard to reference the carve-outs in github because its a
bit confusing.
Merge the carve-outs into a single "one ACK carve-out" with multiple
rules. Use rule 0 for the original refactor carve-out stuff because it
makes the diff smaller and all good lists start with 0.
Also remove mention of the refactor carve-out from rule 3.
Reviewers often find themselves linking to blog posts to encourage newer
devs to improve their commit logs, we can save everyones time by putting
the links in the contributing docs, then we can just point devs there.
We have started introducing `error` sub-modules for modules that have a
lot of error code. This is mainly a code organisation thing.
When importing a function it is annoying to have to go to another module
to get the returned error type. Furthermore it leaks our module
structure and introduces a maintenance burden because for modules that
do not [yet] have an `error` sub-module the error types are in a different
place to modules that do have an `error` sub-module.
Furthermore, if function users have to go to the `error` sub-module to
read docs etc. they are bombarded with all the hider errors as well (see
definitions below).
We can solve both problems by re-exporting all regular errors, making
the `error` sub-module public (for niche users), and not re-exporting
hider errors.
I believe the re-export should only be done in the module that holds
`error` sub-module i.e., do not re-export `foo::error::SomeError` from
`bar` module even if `bar::some_function` returns `SomeError`.
Definitions:
- hider error: An error type, often a struct, that is nested in another
error just to hide the internals.
- regular error: An error type that is returned from a public function,
most often an enum.
Please note that some times errors act as both hider errors and regular
errors so the definitions are not perfect.
These are not run in CI since #2353 and are likely to go out of date. If
we want a script that users can run locally then we should create a new
script that wraps our current CI.
cb42c74f58 policy: Add section on standard set of derives (Tobin C. Harding)
ec1a5a25c7 CONTRIBUTING: Remove stale links (Tobin C. Harding)
Pull request description:
We can have a standard set of derives to make it easier for new devs to work out what to use and also to help us move towards a uniform set in preparation for crate stabilization.
This is not me imposing my view but rather a place for the discussion to happen, could have been a discussions topic also? Using "open" instead of "draft" to aid visibility. Probably requires more than the usual amount of acks.
ACKs for top commit:
apoelstra:
ACK cb42c74f58
Kixunil:
ACK cb42c74f58
Tree-SHA512: b4c4094ea3652e92a5bea90e16f13971202710166524cc15abef5e8318ba5f59df084f5246331870fc641456b49d4e35d266c937375bdc5035f03699a7d4c1b9
We can have a standard set of derives to make it easier for new devs to
work out what to use and also to help us move towards a uniform set in
preparation for crate stabilization. Mention the new `ordered` crate.
3bebecc7ea Add policy section to docs (Tobin C. Harding)
Pull request description:
In an effort to consolidate knowledge spread out over time in various places on GitHub add a `Policy` section to `CONTRIBUTING.md`.
Add initial sections on import statements, errors, rustdocs,attributes, and licensing.
ACKs for top commit:
apoelstra:
ACK 3bebecc7ea
Kixunil:
ACK 3bebecc7ea
Tree-SHA512: fb1afd220a0afae962cd9c7a01df7d440eaa1406b446765af209e3b7840b36aa8a1254d57f9ff0c3783479111470828da3c76b5858ce5969519c96cdba71c7f3
In an effort to consolidate knowledge spread out over time in various
places on GitHub add a `Policy` section to `CONTRIBUTING.md`.
Add initial sections on import statements, errors, rustdocs,attributes,
and licensing.
b229fd5555 Use correct form for noun: carve-out (Tobin C. Harding)
Pull request description:
One "carves out" something, not "carve output" - woops.
ACKs for top commit:
apoelstra:
ACK b229fd5555
Tree-SHA512: 196c5c239da35a67b83eb96f2f9346af6518af51ff6bb4a9e40cddab8c8511f2e913278a9727bfe4e97d50c58cb87a4100e40c13c8313643b04a2d0fabaaa9e5
In an effort to reduce review burden add so strict rules defining 3
times where we think it is acceptable for code to be merged with a
single ACK. Feels a bit like we only just added the refactor carve out
and now we are adding more carve outs, so whats next? We should take
these rules seriously, if we are to be taken seriously.
I have managed to burn out or bore our reviewers/maintainers. Getting
two acks is becoming increasingly difficult. I've pestered everyone to
the limit that I feel socially comfortable doing so am requesting a
carve out to the 2-ACK before merge rule.
The primary justification is that I feel we should have a bit more of
BDFL and a bit less total consensus if we are to push forwards.
It could happen that we unknowingly depend on a new version of a crate
without updating `Cargo.toml`. This could cause resolution issues for
downstream users. It's also unclear for outsiders to see which
dependencies we tested the crate with.
This change commits two lock files: `minimal` and `recent`. `minimal`
contains minimal depdendency versions, while `recent` contains
dependency versions at the time of making the change.
Further, this adds CI jobs to test with both lock files, CI job for
`internals` crate, removes old `serde` pinning and prints a warning if
`recent` is no longer up to date. (We may have to override it somehow if
any crate breaks MSRV.)
The documentation is also updated accordingly.
Co-developed-by: Tobin C. Harding <me@tobin.cc>
Closes#1230
b7eea6cb26 Update docs on rustfmt (Tobin C. Harding)
Pull request description:
We have introduced `rustfmt` but forgot to update the docs section about
it. Since a large portion of the codebase is currently ignored by our
`rustfmt` configuration, point out that `rusntfmt` is work in progress.
ACKs for top commit:
apoelstra:
ACK b7eea6cb26
Kixunil:
ACK b7eea6cb26
Tree-SHA512: c3a01e38e9787f7554847c657de8d2aeb512f237f68e97973ffa04e92decd282126abf16edb369c13dbeb9eed3587e20dbfb3ab77e06551a006a7aa2c70a71ad
We have introduced `rustfmt` but forgot to update the docs section about
it. Since a large portion of the codebase is currently ignored by our
`rustfmt` configuration, point out that `rusntfmt` is work in progress.
We have docs that use an env var `BITCOIN_MSRV` that no longer exists.
Point readers towards `RUSTUP_TOOLCHAIN`.
(Note that we still use `TOOLCHAIN` in the script but it is being phased
out, `RUSTUP_TOOLCHAIN` works today.)
We have stale docs referring to the old MSRV. We do not need MSRV docs
in `CONTRIBUTING` because we have them in the README already.
Fix stale docs by doing:
- Remove the MSRV docs from readme in favour of `CONTRIBUTING.md`.
- Add a sentence to the redame pointing readers towards `CONTRIBUTING.md`.
costcould
jeremiah
Jose Storopoli
Tobin C. Harding
Steven Roose
Jamil Lambert, PhD
Jose Storopoli
Andrew Poelstra
Liam Aharon
Vojtěch Toman
Einherjar
Martin Habovstiak