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

Replace base64-compat dependency #993

Merged
merged 1 commit into from Jun 1, 2022

Conversation

tcharding
Copy link
Member

Now that we have MSRV 1.41.1 we can use the more modern base64 instead of the compat crate. Requires no changes other than changing the dependency.

Now that we have MSRV 1.41.1 we can use the more modern `base64` instead
of the compat crate. Requires no changes other than changing the
dependency.
Copy link
Member

@elichai elichai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 38c41e4

@apoelstra
Copy link
Member

I'm a little wary of doing this. The technical problem with base64 is solved but AFAIK not the social one, which is that the maintainer of base64 feels it's reasonable for a low-level widely-used crate to impose new MSRV requirements on its users in minor releases.

OTOH, since we blacklisted base64 we later discovered the cargo update --precise trick, so I suppose if this happens again we can just update our own README.

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.

ACK 38c41e4

I think, since serde broke MSRV way back when, we need to accept that we're unable to NIH everything that annoys us, and the cargo update -p is honestly not that bad.

@tcharding
Copy link
Member Author

OTOH, since we blacklisted base64 we later discovered the cargo update --precise trick, so I suppose if this happens again we can just update our own README.

Anything can break at anytime, right? Perhaps what we need is to defend against it by making sure our CI build/test against explicit versions so we know what works? This seems to be a recurring theme at the moment, I don't personally know an easy way to fix it? While we are at it I suppose we should discuss auditing specific dependency versions, has that been discussed/proposed before?

A testing epic is probably in order, I'm happy to work on it but probably after crate-smashing is done would be best?

@apoelstra
Copy link
Member

I think it's reasonable to assume that every minor dependency will work, until it doesn't (in which case we'll need to add a pin and update our CI).

Pre-emptively pinning means that we're no longer testing the latest versions of dependencies, which are what our users are most likely to be using.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 38c41e4

@sanket1729 sanket1729 merged commit 68dd23d into rust-bitcoin:master Jun 1, 2022
@tcharding tcharding deleted the 05-11-base64-dep branch July 20, 2022 01:39
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
38c41e4 Replace base64-compat dependency (Tobin C. Harding)

Pull request description:

  Now that we have MSRV 1.41.1 we can use the more modern `base64` instead of the compat crate. Requires no changes other than changing the dependency.

ACKs for top commit:
  elichai:
    ACK 38c41e4
  apoelstra:
    ACK 38c41e4
  sanket1729:
    ACK 38c41e4

Tree-SHA512: 3b53f7c52c9f8346fe4a958b8a8ffa5312891cbb4ce9f5e413bcad596f416ad2f5d6bbbde8857795544de06eaaa2450e88dde273e3177da918baed264a38d1ec
@Kixunil Kixunil added the API break This PR requires a version bump for the next release label Aug 1, 2022
@Kixunil Kixunil added this to the 0.29.0 milestone Aug 1, 2022
@Kixunil
Copy link
Collaborator

Kixunil commented Aug 1, 2022

Noticed this comment while doing pre-release check of PRs:

but AFAIK not the social one, which is that the maintainer of base64 feels it's reasonable for a low-level widely-used crate to impose new MSRV requirements on its users in minor releases.

Doing MSRV bumps in minor releases is actually correct because forcing people to do cargo update -p is less bad than causing ecosystem breakage with major versions. I even believe it's OK to do in patch releases (as long as there's no new API ofc).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API break This PR requires a version bump for the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants