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

Enable edition 2018 #34

Merged
merged 3 commits into from May 18, 2022
Merged

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Jan 15, 2022

Update the MSRV to Rust 1.41.1 and enable edition 2018.

Should not be merged without rust-bitcoin organization-wide planning on how to go about this upgrade.

Leaving as draft until rust-bitcoin Taproot release is done.

Discussion: rust-bitcoin/rust-bitcoin#510 (comment)

@tcharding
Copy link
Member Author

Now includes version bump, please see #40 for discussion on the version format.

@tcharding tcharding marked this pull request as ready for review April 21, 2022 02:25
apoelstra added a commit to rust-bitcoin/bitcoin_hashes that referenced this pull request May 3, 2022
abb7c80 Bump version 0.10.0 -> 0.11.0 (Tobin Harding)
b270023 Update to use edition 2018 (Tobin Harding)
e762962 Update MSRV in CI and Readme from 1.29 to 1.41.1 (Tobin Harding)
3602c75 Remove trailing whitespace (Tobin Harding)

Pull request description:

  Update the MSRV to Rustn 1.41.1 and enable edition 2018.

  Should not be merged without rust-bitcoin organization-wide planning on how to go about this upgrade.

  Discussion: rust-bitcoin/rust-bitcoin#510 (comment)

  This one is a bit more involved than the same PRs for [rust-bech32](rust-bitcoin/rust-bech32#57) or [rust-bitcoinconcensus](rust-bitcoin/rust-bitcoinconsensus#34).

  The commit message of patch 3:
  ```
  Update to use edition 2018

  Add `edition = "2018"` to the minifest file. In order to get the
  codebase to build cleanly do:

  - Remove usage of `use Hash as HashTrait`, instead use `impl crate::Hash for Hash` and `use Hash as _`.
  - Same for HashEngine (remove EngineTrait).
  - Add `crate::` to import statements and group same level (only did this for crate imports, the rest can wait for rustfmt :)
  - Make test imports uniform, elect to _not_ use `super::*` because it seems cleaner, we are always importing the module we are testing and the same set of traits in each `test` module. Can change if requested.
  ```

  Thanks

ACKs for top commit:
  apoelstra:
    ACK abb7c80

Tree-SHA512: 6e99235075a12a82bc2bb032411eb7d022c650e5288bd1a2891b3d863e093ad9398525c1fba41d5e3fdcb194fcf93b00c6f59ad7681f5404eaeae73f08af2278
@tcharding tcharding requested review from stevenroose and apoelstra and removed request for stevenroose May 12, 2022 23:49
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.

utACK 42a0a18

@tcharding
Copy link
Member Author

Changes in force-push: rebase on master ignoring old appveyor config file, no other changes.

Done in preparation for updating the MSRV across all the rust-bitcoin
crates.

For discussion see:

 rust-bitcoin/rust-bitcoin#510 (comment)
Add `edition = 2018` to the manifest file. No further changes were
necessary to get the code to lint cleanly.
@tcharding
Copy link
Member Author

... and fix up github actions CI job.

@tcharding tcharding requested a review from apoelstra May 16, 2022 01:15
@apoelstra
Copy link
Member

@tcharding are you able to merge this? I'm getting a weird error message from the merge script.

We recently bumped the MSRV, this is a major change and requires a
version bump.

We are using a version format that includes the Bitcoin Core version and
the version for this lib, currently the lib version is a single number.
Instead use typical semvar <Major>.<Minor>.<Patch> for the lib.
@tcharding
Copy link
Member Author

Please note I made a change to the PR (I wanted to force push to run CI without AppVeyor)

Version is now 0.19.0-0.4.0 instead of 0.19.0-4.0.0. This implies the version of this lib is 0.4.0 (before this PR the version implied is 3). I'm going to merge without re-ACK, it can be changed before release if anyone wants to post merge NACK.

@tcharding tcharding merged commit 26a110c into rust-bitcoin:master May 18, 2022
@tcharding
Copy link
Member Author

My first try merging using the merge script. I had to hack it to skip bitcoin/depend from the output of git ls-tree -r HEAD. I don't fully understand what is going on in tree_sha512sum but I guessed that we don't want to include depend/bitcoin because it is a submodule.

@tcharding
Copy link
Member Author

diff --git a/github-merge.py b/github-merge.py
index 2dc820d..3330dc0 100755
--- a/github-merge.py
+++ b/github-merge.py
@@ -162,6 +162,8 @@ def tree_sha512sum(commit='HEAD'):
for line in subprocess.check_output([GIT, 'ls-tree', '--full-tree', '-r', commit]).splitlines():
name_sep = line.index(b'\t')
metadata = line[:name_sep].split() # perms, 'blob', blobid
+        if metadata[1] == b'commit':
+            continue
assert(metadata[1] == b'blob')
name = line[name_sep+1:]
files.append(name)

@tcharding tcharding deleted the edition-2018 branch May 18, 2022 23:59
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.

None yet

2 participants