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

Bump some deps #3394

Merged
merged 2 commits into from
Jul 1, 2023
Merged

Conversation

Rustin170506
Copy link
Member

  1. Bump the openssl v0.10.52 -> v0.10.55 to address the security alert. https://github.com/rust-lang/rustup/security/dependabot
  2. Bump proc-macro2 v1.0.51 -> v1.0.63 to make rustup complile. See Delete use of proc_macro::Span::before/after dtolnay/proc-macro2#391.

@Rustin170506 Rustin170506 changed the title Bump the openssl v0.10.52 -> v0.10.55 Bump some deps Jun 30, 2023
@Rustin170506 Rustin170506 requested a review from rbtcollins June 30, 2023 02:20
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Should we update here as well?

rustup/Cargo.toml

Lines 99 to 104 in 137a99b

[dependencies.openssl]
# Used by `curl` or `reqwest` backend although it isn't imported by our rustup :
# this allows controlling the vendoring status without exposing the presence of
# the download crate.
optional = true
version = "0.10"

@Rustin170506
Copy link
Member Author

Should we update here as well?

I updated it with cargo update -p openssl, so I guess we don't need to change it to 0.10.55 explicitly.
I tested it locally:

➜  rustup git:(master) cargo tree -i -p openssl --target all               
openssl v0.10.55
└── native-tls v0.2.11
    ├── hyper-tls v0.5.0
    │   └── reqwest v0.11.17
    │       └── download v1.26.0 (/Volumes/t7/code/rustup/download)
    │           └── rustup v1.26.0 (/Volumes/t7/code/rustup)
    ├── reqwest v0.11.17 (*)
    └── tokio-native-tls v0.3.1
        ├── hyper-tls v0.5.0 (*)
        └── reqwest v0.11.17 (*)

We only have one version openssl in its dependencies.

Thanks for your review! 💚 💙 💜 💛 ❤️

@Rustin170506 Rustin170506 requested a review from weihanglo July 1, 2023 07:54
@weihanglo
Copy link
Member

Yep but it depends on whether or not we allow openssl older 0.10.55 to be built with rustup. Some pros:

  • Cargo's resolver may sometimes converge to downgrade something, this may help us avoid accidental oversight of a PR that downgrades a version in lockfile.
  • People that build their own rustup would get a guarantee of having the new version with the security update.

The counterargument is obviously we then force everyone use 0.10.55 or newer.

@weihanglo
Copy link
Member

I'd say it must be pretty rare that people build rustup on their own, but still there AFAIK.

@Rustin170506
Copy link
Member Author

  • Cargo's resolver may sometimes converge to downgrade something, this may help us avoid accidental oversight of a PR that downgrades a version in lockfile.

For this case, the CI may help us to avoid this kind of situation.

  • People that build their own rustup would get a guarantee of having the new version with the security update.

We already checked the lock file in our repo. Do you mean users will use it as a package?

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Yes. As a dependency. Cargo usually tries to make them in sync for the manifest and the lockfile. It's not really a big deal. So go ahead!

@Rustin170506
Copy link
Member Author

Yes. As a dependency.

The use of rustup as a dependency should be rare. Thank you for your review.

@Rustin170506 Rustin170506 merged commit 849adb7 into rust-lang:master Jul 1, 2023
@Rustin170506 Rustin170506 deleted the rustin-patch-dep branch July 1, 2023 10:44
@rami3l rami3l mentioned this pull request Jan 29, 2024
1 task
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