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

Dependency hygiene for rustc 1.72.0 #48

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

vkkoskie
Copy link

@vkkoskie vkkoskie commented Aug 29, 2023

Hi there! I also use this crate in a codebase I maintain. >_>

Like the filer of #45, I'd also like to get ahead of the future incompatibility warnings that started on or about rustc 1.71.0 with rustc 1.69.0. Those are fixed in the first series of commits (up to bb41f94) by bumping the version of the amq-protocol crate that was pulling in the problematic nom dependency.

The second series does a general version bump across most of the other dependencies and a few other bits of hygiene intended to push the next time this is needed as far into the future as possible (given that you mentioned the crate is no longer maintained actively in #46).

They're separated to make a clear boundary between what's necessary and what's not, but I doubt there's anything controversial in the extra bits.

Notes:

  • The mio dependency remains at 0.6 despite 0.8 being available. This is because mio-extras appears to have chosen to stay at 0.6, and the jump to 0.7 was fairly significant. mio-extras recommends mio-misc as an 0.7-supporting alternative, but the translation between the two isn't a quick one-to-one like most of the other changes in this PR. I expect this will be the next source of maintenance issues. A partial migration is here.
  • amq-protocol now makes a distinction between the short and long string types from the spec. However, conversions are both infallible From<String> and the bounds are definitely not enforced. Given that the ShortString type is marked as deprecated, I opted to simply allow these conversions to take place as written, and didn't attempt to add new bounds checks (which would also address Routing Key Byte Maximum #43).
  • Though the PR indicates 1.72.0, rustc 1.73.0-beta was actually used for confirmation. The bump to the 2021 Edition makes rustc 1.56.0 a plausible minimum, but I didn't confirm that.
  • Only two backward incompatible changes to the public API were needed. Both are mentioned in the Changelog.

Closes #45

@eievui5
Copy link

eievui5 commented Sep 9, 2023

Hi! I merged this PR into a fork I made. I'm planning on maintaining it for a while if you're interested in using it. Thanks for making these changes :)

@jgallagher
Copy link
Collaborator

Hi! I merged this PR into a fork I made. I'm planning on maintaining it for a while if you're interested in using it. Thanks for making these changes :)

You of course need neither my permission nor my blessing to fork this project, but I'll propose an alternative - is anyone interested in taking over ownership of amiquip itself?

@vkkoskie I hate to put you on the spot (in more ways than one >.>) but I'll defer to you first - if we assume I'll continue to be unacceptably slow to respond to issues and PRs, how would you like to proceed? My preference would be to transfer ownership of this repo to an organization, add a new maintainer, and give them rights both to publish updates to crates.io and add additional maintainers in the future (and remove me, should they choose to!). But if that's not of interest and a fork makes more sense, I'd also be happy to archive this repo and point people to it instead.

@vkkoskie
Copy link
Author

vkkoskie commented Sep 9, 2023

Since this is my PR, I suppose I have right of first refusal ... which I'll take now. I'm also not in a position to take on long term maintenance for this package. I don't mind submitting these sorts of hygiene PRs once every few years to meet my own needs, but I doubt I'd be able to be much more responsive to others' requests.

@eievui5 I will certainly switch to bnuuy now if amiquip gets archived. Thanks! But it sounds like you have the option of not needing to switch names at all. Not only does that appeal to the lazy person in me who doesn't want to change all my use statements, it would also maintain continuity in other places like crates.io. Of course, since I'm tapping out, that's really a discussion for you and @jgallagher to have. Just my 2 cents and personal preference.

@eievui5
Copy link

eievui5 commented Sep 9, 2023

I guess I don't mind taking ownership of this repo; retaining the same name would be a plus for sure (though I'm kinda attached to bnuuy :<).

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.

Old version of dependency nom
3 participants