Opened 6 months ago

Last modified 3 months ago

#27130 needs_review defect

rust dependency updating instructions don't work

Reported by: cyberpunks Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: 0.3.3.9
Severity: Normal Keywords: rust, doc, 033-backport, 034-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: catalyst Sponsor:

Description

None of the instructions mention updating Cargo.lock, which is required. The script updateRustDependencies.sh doesn't update that file, either.

Child Tickets

Change History (15)

comment:1 Changed 6 months ago by cyberpunks

See branch 'rust-dependencies1' at https://gitgud.io/onionk/tor.git

comment:2 Changed 6 months ago by teor

Keywords: 032-backport 033-backport 034-backport added
Milestone: Tor: 0.3.5.x-final
Status: newneeds_review

Hi, thanks for this patch.

Did you just test on master?
Do you think we need to backport this patch to previous releases?
(Rust support was introduced in 0.3.1, and we still support 0.3.2 and later.)

comment:3 Changed 6 months ago by cyberpunks

Yes, though the only real code change is dropping a single argument to cargo-vendor in the shell script.

Backporting would make sense, but the shell script and the instructions in the documentation only exist in 0.3.3 and higher.

comment:4 Changed 6 months ago by teor

Keywords: 032-backport removed

comment:5 Changed 5 months ago by asn

Reviewer: catalyst

comment:6 Changed 5 months ago by catalyst

comment:7 Changed 5 months ago by catalyst

I think we only need to backport if we expect to need to update the rust dependencies in the older releases.

comment:8 in reply to:  7 Changed 5 months ago by teor

Replying to catalyst:

I think we only need to backport if we expect to need to update the rust dependencies in the older releases.

While the older releases are still supported, we might update their dependencies:

  • to resolve rust bugs, or
  • to pick up security fixes, or
  • if the exact versions of crates in the old cargo config stop being available.

It's also worth being consistent between releases.

Since this patch is pretty simple, I think we should backport.

comment:9 Changed 5 months ago by catalyst

Status: needs_reviewneeds_information

It looks like updates to newer versions still won't happen without cargo update right? Should we add that to the instructions? What exactly should we say?

comment:10 in reply to:  9 ; Changed 5 months ago by cyberpunks

Replying to catalyst:

It looks like updates to newer versions still won't happen without cargo update right?

Are you sure? That shouldn't be true, that's the entire point of the code change. It now is allowed to alter the Cargo.lock file. As the code was before, you had to run cargo update beforehand, and the instructions never mentioned this requirement. Now you don't have to at all. Just commit the change it made.

comment:11 in reply to:  10 ; Changed 5 months ago by catalyst

Replying to cyberpunks:

Replying to catalyst:

It looks like updates to newer versions still won't happen without cargo update right?

Are you sure? That shouldn't be true, that's the entire point of the code change. It now is allowed to alter the Cargo.lock file. As the code was before, you had to run cargo update beforehand, and the instructions never mentioned this requirement. Now you don't have to at all. Just commit the change it made.

I just tried it. It looks like I have to run cargo update to update Cargo.lock to a newer version of libc, and cargo vendor doesn't try to change it.

comment:12 in reply to:  11 ; Changed 5 months ago by cyberpunks

Replying to catalyst:

I just tried it. It looks like I have to run cargo update to update Cargo.lock to a newer version of libc, and cargo vendor doesn't try to change it.

Did you make any crate actually require a newer version of libc? That's in the instructions as the first step.

To add/remove/update dependencies, first add your dependencies into the appropriate *crate-level* Cargo.toml

If dependencies are being updated, it would be because there's a reason for it and you now require a new minimum version, right?

Pushed a commit to maybe make the instructions a bit clearer.

Last edited 5 months ago by cyberpunks (previous) (diff)

comment:13 in reply to:  12 ; Changed 5 months ago by catalyst

Replying to cyberpunks:

Replying to catalyst:

I just tried it. It looks like I have to run cargo update to update Cargo.lock to a newer version of libc, and cargo vendor doesn't try to change it.

Did you make any crate actually require a newer version of libc? That's in the instructions as the first step.

Thanks, that seems to have helped.

To add/remove/update dependencies, first add your dependencies into the appropriate *crate-level* Cargo.toml

If dependencies are being updated, it would be because there's a reason for it and you now require a new minimum version, right?

As teor mentioned above, what if an older version of a dependency had bugs or security vulnerabilities? Then we might want the newest compatible version without explicitly bumping the minimum version in Cargo.toml.

Pushed a commit to maybe make the instructions a bit clearer.

Thanks. Could you please also explain why someone might want to manually edit versions in Cargo.toml versus running cargo update?

comment:14 in reply to:  13 Changed 5 months ago by cyberpunks

Changed the instructions around so that changes to Cargo.lock are always done one way, using cargo update, instead of sometimes with cargo update and sometimes with this script, depending.

Also it turns out cargo update doesn't even work unless you remove .cargo/config first, since the settings there leave cargo only able to access the vendored sources and not the real registry. And there's no way to override source-replacements like that with environment variables.

Last edited 5 months ago by cyberpunks (previous) (diff)

comment:15 Changed 3 months ago by catalyst

Status: needs_informationneeds_review
Note: See TracTickets for help on using tickets.