Skip to content

Clean up after subcrate creation#1337

Closed
tcharding wants to merge 5 commits intorust-bitcoin:masterfrom
tcharding:10-22-mv-fuzz
Closed

Clean up after subcrate creation#1337
tcharding wants to merge 5 commits intorust-bitcoin:masterfrom
tcharding:10-22-mv-fuzz

Conversation

@tcharding
Copy link
Copy Markdown
Member

During the recent creation of the bitcoin subcrate I got a few things wrong, not putting embedded, fuzz, and the changelog into the new subcrate. PR also includes updating .gitignore and adding a changelog to internals.

This was originally part of

#1284 but its stand alone work and should be more easily reviewable done separately.

@apoelstra
Copy link
Copy Markdown
Member

concept ACK, though to be clear -- did we land on fuzz/ being a per-crate thing? In an earlier version of #1284 we were mixing fuzztests for bitcoin and hashes. This has something of a benefit in that we can fuzz everything altogether in one CI job, but it also is potentially confusing. I forget where we landed there.

@tcharding
Copy link
Copy Markdown
Member Author

In the discussion thread of #1284 you suggest doing fuzzing per crate, I think I remember coming to the same conclusion myself, though I may be mixing it up with the embedded tests, which are definitely per crate. I can come back and merge all the fuzzing together later on if needed without too much trouble.

apoelstra
apoelstra previously approved these changes Oct 22, 2022
Copy link
Copy Markdown
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 72238fdb6685b2919f1ce108bb89bc42e52c1093

Recently we introduced a workspace but did not update the git ignore
file. Ignore lock files, build artifacts, test artifacts, and fuzz
artifacts.
Recently we added a workspace but left the CHANGELOG file at the
repository root, this is incorrect because the CHANGELOG is a per crate
thing since it is updated along with crate release.
The changelog file is a per crate, per release, thing. Add one to the
`internals` crate.
Fuzzing is a per-crate thing, when we created the `bitcoin` subcrate
fuzzing should have been moved there also.

Move `fuzz` to the `bitcoin` crate, update `.gitignore`, the workspace
manifest, and the `fuzz` CI workflow.
The embedded tests are a per-crate thing, when we created the `bitcoin`
subcrate, the embedded code should have been moved there also.

Move `embedded` to the `bitcoin` crate, update the workspace manifest,
and the `rust.yml` CI workflow.
@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Nov 8, 2022

Re-based to fix non-existent merge conflicts.

Copy link
Copy Markdown
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 7d62159

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Nov 14, 2022

Perhaps don't merge this one, looks like #1284 might go in, it includes all the changes in this PR (although the commits are different, my bad, I can't remember why but I seem to have squashed the last two commits of this one into the second last commit of 1284 (the "import bitcoin_hashes" patch)). Oh I split this one out of 1284 to try to help review.

@tcharding
Copy link
Copy Markdown
Member Author

There is no merge conflict with fuzz/fuzz_targets/deserialize_psbt.rs

apoelstra added a commit that referenced this pull request Nov 18, 2022
9674bf2 hashes: Fix clippy warnings (Tobin C. Harding)
b9643bf Import bitcoin_hashes crate into hashes (Tobin C. Harding)
580feab internals: Add CHANGELOG file (Tobin C. Harding)
bae64e1 Move CHANGELOG to bitcoin crate (Tobin C. Harding)
9a2c856 Update gitignore file (Tobin C. Harding)

Pull request description:

  #1337 was split out of this in an attempt to help review, I think we can merge this one though now it has some traction.

  We would like to bring the `bitcoin_hashes` crate into the `rust-bitcoin` repository. #550 (comment)

  Import `bitcoin_hashes` crate into `hashes`.

  Commit hash that was tip of `bitcoin_hashes` at time of import:

       commit 2a78c250f78d391599040223870a4d1d6f6f5482

  Please note the commit history of `bitcoin_hashes` is only preserved on the soon-to-be-archived `bitcoin_hashes` repository.

ACKs for top commit:
  sanket1729:
    ACK 9674bf2. Did my best to review the CI code, it looks good, and seems like we are testing everything when we add "hashes" to the root level `./contrib/tests.sh`. I would like other reviewers to confirm the same.
  apoelstra:
    ACK 9674bf2

Tree-SHA512: db3ce6adeb38430c5a9f372da16be9fb048c2e5cff646b701139fb4b5fa76e10261432284ede7fd139b75cd69bb124d55973ceb7eccaa996226a7be4cad9b68a
@apoelstra
Copy link
Copy Markdown
Member

Now that #1284 is in, what is the state of this PR?

@tcharding
Copy link
Copy Markdown
Member Author

This one is redundant now, all changes were included in #1284

@tcharding tcharding closed this Nov 18, 2022
@tcharding tcharding deleted the 10-22-mv-fuzz branch November 18, 2022 19:29
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.

2 participants