Opened 14 months ago

Last modified 5 weeks ago

#27130 accepted defect

rust dependency updating instructions don't work

Reported by: cyberpunks Owned by: nickm
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.3.3.9
Severity: Normal Keywords: rust, doc, 033-unreached-backport, 042-deferred-20190918
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 (25)

comment:1 Changed 14 months ago by cyberpunks

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

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

Keywords: 032-backport removed

comment:5 Changed 13 months ago by asn

Reviewer: catalyst

comment:6 Changed 13 months ago by catalyst

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

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

comment:15 Changed 11 months ago by catalyst

Status: needs_informationneeds_review

comment:16 Changed 8 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 8 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 8 months ago by teor

Keywords: 033-backport-unreached added

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

comment:19 Changed 6 months ago by catalyst

Reviewer: catalystnickm

comment:20 Changed 5 months ago by nickm

Mark a few documentation tickets as "doc"

comment:21 Changed 5 months ago by nickm

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

comment:22 Changed 4 months ago by nickm

Keywords: 034-backport removed

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

comment:23 Changed 2 months ago by teor

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

Fix 033-unreached-backport spelling.

comment:24 Changed 8 weeks ago by nickm

Owner: set to nickm
Status: needs_reviewaccepted

Trying to pick this up again.

Looking at the branch, I see several changes.

  1. It makes the updateRustDependencies.sh script exit on error.
  2. It changes the crate dependency versions listed in our Cargo.toml files to be no longer pinned, under the theory that Cargo.lock is the correct mechanism for pinning versions.
  3. It updates the instructions for updating versions so that:
    • they no longer recommend version pinning with Cargo.toml
    • they give more information about the correct sequence of action in git.


I agree with 1 and 3; we should discuss 2 as a team.

When I try to use this script, I see two problems:

First, cargo now includes "cargo vendor", but it has been updated to no longer support the --explicit-version flag. We should investigate why, and what to do about it.

Second, I get: "./scripts/maint/updateRustDependencies.sh: line 51: test: too many arguments". This is an easy fix.

My next step here is to read up on why --explicit-version is no longer supported here, and to start a discussion on what we actually want to do about versions.

comment:25 Changed 5 weeks ago by nickm

Keywords: 042-deferred-20190918 added
Milestone: Tor: 0.4.2.x-finalTor: unspecified

Deferring various tickets from 0.4.2 to Unspecified.

Note: See TracTickets for help on using tickets.