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

chore: update dependencies to newer versions #740

Closed
wants to merge 1 commit into from

Conversation

ctron
Copy link
Contributor

@ctron ctron commented Jul 9, 2024

Mainly to update jsonschema -> reqwest -> hyper to a 1.x version.

@ctron ctron requested a review from a team as a code owner July 9, 2024 13:57
@ctron ctron force-pushed the feature/update_deps_1 branch 2 times, most recently from cd2585d to a17ed16 Compare July 9, 2024 14:40
@ctron
Copy link
Contributor Author

ctron commented Jul 9, 2024

Sorry, but I have no idea what to do about that nix flake issue.

@Shnatsel
Copy link
Contributor

Shnatsel commented Jul 9, 2024

I think a better idea is to just disable the resolve* features that pull in hyper. We really don't want to pull in the entirety of reqwest, hyper and tokio unconditionally from a serialization/deserialization crate.

Also that's definitely a no on the packageurl upgrade - that's certainly semver-breaking, and if we're breaking semver we should just switch to the purl crate which is much better all around.

@ctron
Copy link
Contributor Author

ctron commented Jul 9, 2024

I think a better idea is to just disable the resolve* features that pull in hyper. We really don't want to pull in the entirety of reqwest, hyper and tokio unconditionally from a serialization/deserialization crate.

Also that's definitely a no on the packageurl upgrade - that's certainly semver-breaking, and if we're breaking semver we should just switch to the purl crate which is much better all around.

Ok, I'll try to update the PR tomorrow.

@Shnatsel
Copy link
Contributor

The jsonschema crate pulls in a lot of dependencies and shouldn't be a runtime dependency at all. We should turn it into a dev-dependency. There is now an issue about it: #741

@ctron
Copy link
Contributor Author

ctron commented Jul 11, 2024

Ok, I changed it to the following:

  • packageurl is back to 0.3
  • the jsonschema is an optional dependency, enabled by default. As the validate method is a public method, that would break the API. This can be turned into off-by-default with the next breaking release.
  • Also, jsonschema has default-features=false, which is sufficient for this crate

Mainly to update jsonschema -> reqwest -> hyper to a 1.x version.

Also, this allows to disable the jsonschema dependency. Currently, it is
enabled by default to be compatible with previous versions. But this
can be changed in the next breaking release to exclude it by default.

Signed-off-by: Jens Reimann <ctron@dentrassi.de>
@Shnatsel
Copy link
Contributor

Shnatsel commented Jul 11, 2024

Turning jsonschema into an optional feature is a breaking change if someone was using the crate with default-features = false, but I don't think anyone has been doing that on account of the crate not having any features. So that sounds like a reasonable short-term fix.

The Nix flake CI is failing due to increased MSRV. It expects 1.70, while this PR bumps it to 1.74. 1.74 is very recent, and I think we would like to stick to 1.70 for now.

What is the motivation for the upgrades? We could consider bumping it if it's really worth it.

@Shnatsel
Copy link
Contributor

Since this appears to have stalled, I started pruning the dependency tree myself. See #744 and #746

@ctron
Copy link
Contributor Author

ctron commented Jul 16, 2024

Since this appears to have stalled, I started pruning the dependency tree myself. See #744 and #746

Sorry for that. Yes, that might be quicker.

@Shnatsel
Copy link
Contributor

Shnatsel commented Aug 6, 2024

We've dropped reqwest entirely in #744 and #750, so I'm going to go ahead and close this.

@Shnatsel Shnatsel closed this Aug 6, 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.

2 participants