Opened 2 years ago

Last modified 6 months 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 (27)

comment:1 Changed 2 years ago by cyberpunks

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

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

Keywords: 032-backport removed

comment:5 Changed 2 years ago by asn

Reviewer: catalyst

comment:6 Changed 2 years ago by catalyst

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

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

comment:15 Changed 2 years ago by catalyst

Status: needs_informationneeds_review

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

Keywords: 033-backport-unreached added

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

comment:19 Changed 18 months ago by catalyst

Reviewer: catalystnickm

comment:20 Changed 17 months ago by nickm

Mark a few documentation tickets as "doc"

comment:21 Changed 17 months ago by nickm

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

comment:22 Changed 17 months ago by nickm

Keywords: 034-backport removed

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

comment:23 Changed 15 months ago by teor

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

Fix 033-unreached-backport spelling.

comment:24 Changed 14 months 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 13 months 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.

comment:26 in reply to:  24 ; Changed 6 months ago by cypherpunks

Replying to nickm:

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

So has there been any team discussion of 2 yet?

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.

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.

What has reading up on this turned up?

cargo vendor was merged in cargo 0.38 which came out August 15, 2019. (So if you tested this script in the 10 months between when the branch was last reviewed in September 2018 and July 2019, you wouldn't have had any issues.)

All tests were imported as part of this commit, but not all features
were imported. Some flags have been left out that were added later in
the lifetime of cargo vendor which seem like they're more questionable
to stabilize. I'm hoping that they can have separate PRs adding their
implementation here, and we can make a decision of their stabilization
at a later date.

The original flag simply wasn't reimplemented. Until it was, several months later, with a new name: --versioned-dirs. That was released in cargo 0.43, which came out in March 2020.

comment:27 in reply to:  26 Changed 6 months ago by teor

Replying to cypherpunks:

cargo vendor was merged in cargo 0.38 which came out August 15, 2019. (So if you tested this script in the 10 months between when the branch was last reviewed in September 2018 and July 2019, you wouldn't have had any issues.)

Some of our developer-only scripts aren't used that frequently. And we haven't been doing much Rust work lately.

Is there a way to test this script in CI?

(The most reliable way for us to make sure that a test happens is to run it on every commit.)

As a first step, we could run the script against whatever happens to be on crates.io, and make sure it passes whether there are new crates or not.

In future, we may want to be notified when there are new versions of a crate.

Note: See TracTickets for help on using tickets.