Opened 12 months ago

Last modified 13 days ago

#27130 needs_review defect

rust dependency updating instructions don't work

Reported by: cyberpunks Owned by:
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: 0.3.3.9
Severity: Normal Keywords: rust, doc, 033-unreached-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm 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 (23)

comment:1 Changed 12 months ago by cyberpunks

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

comment:2 Changed 12 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 12 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 12 months ago by teor

Keywords: 032-backport removed

comment:5 Changed 11 months ago by asn

Reviewer: catalyst

comment:6 Changed 11 months ago by catalyst

comment:7 Changed 11 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 11 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 11 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 11 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 11 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 11 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 11 months ago by cyberpunks (previous) (diff)

comment:13 in reply to:  12 ; Changed 11 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 11 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 11 months ago by cyberpunks (previous) (diff)

comment:15 Changed 9 months ago by catalyst

Status: needs_informationneeds_review

comment:16 Changed 6 months ago by teor

Milestone: Tor: 0.3.5.x-finalTor: 0.4.1.x-final

Hi catalyst, what's the status of this branch?
Is it still worth merging?
(If it is, we will merge it into 0.4.1 first.)

comment:17 Changed 6 months ago by teor

Keywords: 033-backport removed

These open, non-merge_ready tickets can not get backported to 0.3.3, because 0.3.3 is now unsupported.

comment:18 Changed 6 months ago by teor

Keywords: 033-backport-unreached added

Hmm, I guess they should still get 033-backport-unreached

comment:19 Changed 4 months ago by catalyst

Reviewer: catalystnickm

comment:20 Changed 3 months ago by nickm

Mark a few documentation tickets as "doc"

comment:21 Changed 3 months ago by nickm

Milestone: Tor: 0.4.1.x-finalTor: 0.4.2.x-final

comment:22 Changed 2 months ago by nickm

Keywords: 034-backport removed

Removing 034-backport from all open tickets: 034 has reached EOL.

comment:23 Changed 13 days ago by teor

Keywords: 033-unreached-backport added; 033-backport-unreached removed

Fix 033-unreached-backport spelling.

Note: See TracTickets for help on using tickets.