Opened 6 weeks ago

Closed 5 weeks ago

#22830 closed defect (fixed)

Problems with building with --enable-rust with RUST_DEPENDENCIES

Reported by: isis Owned by: chelseakomlo
Priority: High Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version: Tor: 0.3.1.1-alpha
Severity: Normal Keywords: rust, tor-build
Cc: chelseakomlo, acrichton@… Actual Points:
Parent ID: Points: 1
Reviewer: isis Sponsor: SponsorZ

Description (last modified by isis)

From #22816, Chelsea and I realised that there are some problems when trying to build the Rust code currently in tor in "offline mode" with RUST_DEPENDENCIES specifying a directory where the dependencies should live. (I.e. building with RUST_DEPENDENCIES='path_to_dependencies_directory' ./configure --enable-rust as noted on the wiki page on building with Rust.)

So one problem was that I had a relative path for RUST_DEPENDENCIES (now noted on the wiki that it must be the full path).

There are other problems though. Cargo expects a lot of structure to be there (basically everything that would normally be in ~/.cargo needs to be in the directory we're giving it as our HOME or CARGO_HOME). While we could write some scripts or something to basically reimplement a bunch of cargo functionality to set all that up, it's kind of non-trivial and it seems like a potential maintainability hazard. Another, even worse way (IMHO) around this would be to say, "if you use RUST_DEPENDENCIES then the entire build doesn't use cargo, but instead calls rustc with the flags that cargo would have." (This seems even less maintainable, because then we have two distinct ways to compile the Rust code.)

Instead, I think a solution which is potentially better than either of those would be to say something like: "If you want to use RUST_DEPENDENCIES, you have to run the following first, which will use cargo to get the dependencies, and then you can re-use the directory it produces offline by specifying it in `RUST_DEPENDENCIES."

mkdir path_to_dependencies_directory
cd src/rust
rm .cargo/config
CARGO_HOME="/whatever/full/path_to_dependencies_directory/.cargo" cargo update
CARGO_HOME="/whatever/full/path_to_dependencies_directory/.cargo" cargo fetch

After this initial set up of the directories which cargo is expecting the normal offline build should be conducted, and instead of passing HOME to cargo, we should pass CARGO_HOME as is used above.

Child Tickets

Change History (18)

comment:1 Changed 6 weeks ago by isis

Description: modified (diff)

comment:2 Changed 6 weeks ago by isis

After reading a discussion around similar issues on the cargo issue tracker, I'm more convinced that the "let cargo run once and then let it run in offline mode" solution is the better option to go with.

comment:3 in reply to:  2 ; Changed 6 weeks ago by gk

Replying to isis:

After reading a discussion around similar issues on the cargo issue tracker, I'm more convinced that the "let cargo run once and then let it run in offline mode" solution is the better option to go with.

If I understand it correctly this means we could let run cargo during the phase in the Gitian build where all the dependencies are fetched and would copy over the result to the build VM (along with the tor source code) and could do the building itself offline then, right? If so, sounds good to me.

comment:4 in reply to:  3 Changed 6 weeks ago by isis

Replying to gk:

Replying to isis:

After reading a discussion around similar issues on the cargo issue tracker, I'm more convinced that the "let cargo run once and then let it run in offline mode" solution is the better option to go with.

If I understand it correctly this means we could let run cargo during the phase in the Gitian build where all the dependencies are fetched and would copy over the result to the build VM (along with the tor source code) and could do the building itself offline then, right? If so, sounds good to me.


Yes, this appears to be the case. That's a good point that this model already works well with Gitian.

comment:5 Changed 6 weeks ago by dgoulet

Milestone: Tor: unspecified

comment:6 Changed 5 weeks ago by chelseakomlo

It seems there are two changes needed to close this ticket, but let me know if these are not what you were thinking:

  1. We should update documentation for how to build in offline mode, via instructions above.
  2. We should use CARGO_HOME rather than HOME when building with Rust (see src/rust/tor_util/include.am, for example)

comment:7 in reply to:  6 Changed 5 weeks ago by isis

Replying to chelseakomlo:

It seems there are two changes needed to close this ticket, but let me know if these are not what you were thinking:

  1. We should update documentation for how to build in offline mode, via instructions above.
  2. We should use CARGO_HOME rather than HOME when building with Rust (see src/rust/tor_util/include.am, for example)


Yep! While working on #22830 I also verified that switching to CARGO_HOME fixes the build.

comment:8 Changed 5 weeks ago by chelseakomlo

Owner: set to chelseakomlo
Status: newassigned

comment:9 Changed 5 weeks ago by isis

Reviewer: isis

comment:10 in reply to:  6 Changed 5 weeks ago by chelseakomlo

Replying to chelseakomlo:

It seems there are two changes needed to close this ticket, but let me know if these are not what you were thinking:

  1. We should update documentation for how to build in offline mode, via instructions above.

I can't reproduce the instructions you have in the ticket description for offline builds. Do you mind writing these up (if there is any more detail required)? Specifically, I'm having trouble with crate versions- cargo update wants to pull the latest libc version (0.2.26), rather than 0.2.22 which we require.

  1. We should use CARGO_HOME rather than HOME when building with Rust (see src/rust/tor_util/include.am, for example)

See git@github.com:chelseakomlo/tor_patches.git, branch bug22830

Let me know if anything else is required for offline builds.

I also specified the libc version, as this is what we require when checking for rust dependencies.

comment:11 Changed 5 weeks ago by chelseakomlo

Status: assignedneeds_review

comment:12 Changed 5 weeks ago by alexcrichton

Hello! I may not be following this issue 100%, but if this is largely targeted at getting offline builds working I'd recommend pursuing #22907 first rather than munging with CARGO_HOME and various builds.

In general though Cargo provides the guarantee that if you do something like cargo fetch then if you don't change Cargo.toml it'll never touch the network. The cargo fetch step just fills in global caches/indexes/whatnot.

What tools like cargo vendor do is provide the guarantee that you don't even need to run cargo fetch and such. The vendor directory created is sufficient to suffice for all of Cargo's networking needs. You can find some more info about this online at http://doc.crates.io/specifying-dependencies.html as well.

As a final note, if you're specifically configuring CI here I'd recommend either the --frozen or --locked flags to Cargo. Cargo typically automatically updates Cargo.lock whenever you update Cargo.toml, for example if Cargo.lock is pinned to libc 0.2.24 and then you update the manifest to libc = "0.2.25" then Cargo will automatically update the lock file. This is undesirable, however, for CI b/c you want the lock file updated for review, so the --locked flag to Cargo says "generate an error if the lock file needs to be updated".

The --frozen flag is a superset of the --locked flag where it also tells Cargo to generate an error if it otherwise would hit the network. This is a way of saying "I assert that Cargo shouldn't touch the network" and is often useful for debugging offline builds.

comment:13 Changed 5 weeks ago by alexcrichton

Cc: acrichton@… added

comment:14 Changed 5 weeks ago by chelseakomlo

Status: needs_reviewneeds_information

comment:15 Changed 5 weeks ago by chelseakomlo

Agreed that investigating #22907 first is a good idea, it would be nice to not have to re-invent the wheel with this!

Tracking specifying libc versions in #22905; not having wildcard versions is a good policy to follow in general.

comment:16 Changed 5 weeks ago by isis

Milestone: Tor: unspecifiedTor: 0.3.1.x-final
Status: needs_informationmerge_ready
Version: Tor: 0.3.1.1-alpha

Hi! This looks great! There's two commits here, the first of which fixes this bug, and the second fixes #22905, so I've gone and split them into two branches and added changes files.

The branch for this ticket is the bug22830 branch in my repo, and contains chelseakomlo's fix plus the changes file.

This should possibly be back-ported to 0.3.1.x-final, since it's a trivial fix and the rust builds are broken without it. My branch for that is bug22830_0.3.1.

comment:17 in reply to:  15 Changed 5 weeks ago by isis

Replying to chelseakomlo:

Agreed that investigating #22907 first is a good idea, it would be nice to not have to re-invent the wheel with this!


Agreed that it would be better to not munge CARGO_HOME and investigate #22907. Let's do that there, and merge the tiny fix here so that at least the builds complete, even if the offline build infrastructure isn't the best.

Tracking specifying libc versions in #22905; not having wildcard versions is a good policy to follow in general.


Also agreed.

comment:18 Changed 5 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged to 0.3.1 and forward. Thanks!

(FWIW, I am giving rust build stuff only the most cursory of overviews here.)

Note: See TracTickets for help on using tickets.