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

remove the direct dependency to quinn-udp #1935

Merged
merged 1 commit into from
May 17, 2023
Merged

remove the direct dependency to quinn-udp #1935

merged 1 commit into from
May 17, 2023

Conversation

zh-jq
Copy link
Contributor

@zh-jq zh-jq commented May 16, 2023

The cargo.lock file is also updated

Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

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

We should actually just remove this dependency -- #1916 switched to using the quinn::udp re-export instead, so we no longer need to depend on this directly.

@zh-jq zh-jq changed the title update to use quinn-udp 0.4 remove the direct dependency to quinn-udp May 16, 2023
@djc
Copy link
Collaborator

djc commented May 16, 2023

What was your reason to update the Cargo.lock file, other than just propagating the quinn-udp removal? It seems odd to me that some dependencies were removed in #1930 only to be added back here.

@zh-jq
Copy link
Contributor Author

zh-jq commented May 17, 2023

The file is auto updated by cargo update, and the CI passed by only downgrading time crates, so is there any reason to keep an old cargo.lock?

@bluejekyll
Copy link
Member

If it passes the MSRV checks and all the other tests, I'm ok with the update to the lock file personally... that's really just a check for people to know all the versions which the project has tested against and are known to be "good".

@zh-jq
Copy link
Contributor Author

zh-jq commented May 17, 2023

that's really just a check for people to know all the versions which the project has tested against and are known to be "good".

I restored the old Cargo.lock file now. But I don't think this will make sense for library users.

@djc
Copy link
Collaborator

djc commented May 17, 2023

I restored the old Cargo.lock file now. But I don't think this will make sense for library users.

Library users aren't affected by the Cargo.lock file, at all, so there's no need to do anything for their use cases.

If it passes the MSRV checks and all the other tests, I'm ok with the update to the lock file personally... that's really just a check for people to know all the versions which the project has tested against and are known to be "good".

If this was the policy, we could have random Cargo.lock updates in every PR. IMO that would make changes harder to review, so I prefer to deal with lockfile updates separately from other changes.

@bluejekyll
Copy link
Member

Thanks, @djc we can set a policy for Cargo.lock updates being separate. I think we could at that to CONTRIBUTING, I’ll try and find some time to add that explicitly in there.

Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@bluejekyll bluejekyll merged commit 41b6e33 into hickory-dns:main May 17, 2023
17 checks passed
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

3 participants