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

Update deny config & bump base64 to 0.22 #119

Merged
merged 4 commits into from
Aug 11, 2024
Merged

Update deny config & bump base64 to 0.22 #119

merged 4 commits into from
Aug 11, 2024

Conversation

djc
Copy link
Member

@djc djc commented Aug 5, 2024

@djc djc requested review from cpu and complexspaces August 5, 2024 12:02
@djc
Copy link
Member Author

djc commented Aug 5, 2024

Not sure what's going on with the Windows CI jobs here.

@cpu
Copy link
Member

cpu commented Aug 5, 2024

Not sure what's going on with the Windows CI jobs here.

I'm not sure either, but it's unrelated to this branch. I opened #117 for the failures the other day but haven't investigated yet.

Cargo.lock Outdated
Comment on lines 44 to 43
[[package]]
name = "base64"
version = "0.22.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "72b3254f16251a8381aa12e40e3c4d2f0199f8c6508fbecb9d91f575e0fbb8c6"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: Why are we bumping base64 when it just duplicates it in-tree compared to the rest of the lower-level rustls crates?

Copy link
Member Author

Choose a reason for hiding this comment

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

It has been updated in the upstream crates, but we haven't been merging Cargo.lock updates as proposed by Dependabot in this repo.

Copy link
Member

Choose a reason for hiding this comment

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

we haven't been merging Cargo.lock updates as proposed by Dependabot in this repo.

In retrospect I think turning that on for this repo was probably a mistake. I'd be OK with removing it again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do you think so?

Copy link
Member

Choose a reason for hiding this comment

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

Why do you think so?

The update PRs sit without reviews/merge and so the bot updates just become churn notifications.

From my perspective the norms for reviewing those PRs hasn't been set in this repo, or written down explicitly. I personally don't feel enough ownership here to blanket +1 updates that don't affect MSRV like I might in rustls/rustls or rustls/webpki.

I remember @complexspaces pushing back on landing the automation and in retrospect I think it would have been better to work through some kind of policy/expectation-setting before landing the automation. Lesson learned :-)

Copy link
Member

Choose a reason for hiding this comment

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

I'd be OK with removing it again

Here's a PR for that: #120

If nothing else it gives us a place to continue this discussion separate from the cargo deny issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I hope no one feels bad about the current state of the lockfile/dependency updates, that wasn't my intention at all. I tend to not look at the PRs because my OSS time is already fairly limited.

I did push back when first proposed because it seemed weird to me to update dependencies via lockfile just because they had updated, since this is generally not what I do at work or personal projects. I tend to only do dependency upgrades when there's a new direct dependency and then make sure the lockfile is clean as an end result.

This is definitely something I'd be happy discussing more though, since I don't think there's one correct way to do this across all projects.

Copy link
Member

Choose a reason for hiding this comment

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

I hope no one feels bad about the current state of the lockfile/dependency updates

No hard feelings here!

Copy link
Member Author

Choose a reason for hiding this comment

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

Nor here!

Copy link
Member Author

Choose a reason for hiding this comment

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

@complexspaces I've tacked on a separate commit here which just did cargo update, which shows that this crate only depends on base64 0.22 after these changes.

@djc
Copy link
Member Author

djc commented Aug 5, 2024

So I guess one potential benefit of keeping Cargo.lock up to date is that we will notice sooner when upstream dependencies adopt a newer MSRV, which might affect our MSRV (although this is potentially not such a big deal for semver-compatible updates).

@cpu
Copy link
Member

cpu commented Aug 6, 2024

@djc Can you rebase this on main for the test data fixes?

@complexspaces Anything else you want to see done with this branch? It'd be nice to have passing CI for community contributions like #122

@djc
Copy link
Member Author

djc commented Aug 7, 2024

Rebased!

@complexspaces
Copy link
Collaborator

Anything else you want to see done with this branch?

Nope, things look good now 👍. Sorry, it was a busy week.

@complexspaces complexspaces merged commit 11201c0 into main Aug 11, 2024
20 checks passed
@djc djc mentioned this pull request Aug 19, 2024
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.

4 participants