Opened 3 months ago

Closed 5 weeks ago

Last modified 5 weeks ago

#26497 closed defect (fixed)

set CARGO_HOME correctly or stop using it

Reported by: catalyst Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: 0.3.1.5-alpha
Severity: Normal Keywords: rust, tor-ci, 035-removed-20180711, 035-roadmap-proposed
Cc: Actual Points:
Parent ID: #24629 Points:
Reviewer: catalyst Sponsor:

Description

It looks like we set CARGO_HOME incorrectly for invocations of both cargo build and cargo test. The cargo build invocations work because we first cd to the directory containing the .cargo directory. The cargo test invocations don't work for out-of-source-tree builds. #26455 contains a more minimal fix.

The setting of CARGO_HOME is redundant if we cd to the correct directory first. Or we could stop changing directories and instead set CARGO_HOME correctly. We probably shouldn't do both because it's confusing and possibly inconsistent.

Child Tickets

Change History (14)

comment:1 in reply to:  description Changed 3 months ago by catalyst

Replying to catalyst:

The setting of CARGO_HOME is redundant if we cd to the correct directory first. Or we could stop changing directories and instead set CARGO_HOME correctly. We probably shouldn't do both because it's confusing and possibly inconsistent.

These are the reasons I can think of to set CARGO_HOME during our build:

  • so cargo finds the correct .cargo/config file; this can be worked around by doing a cd to the correct directory
  • so cargo uses the desired cache directory for online crates if we're configured with --enable-cargo-online-mode; we might not actually want to do this (see #26038)
  • are there other reasons I'm missing here?

I looked briefly at the source code of cargo and it seems to be consistent with the above.

comment:2 Changed 3 months ago by nickm

Keywords: 035-removed-20180711 added
Milestone: Tor: 0.3.5.x-finalTor: unspecified

These tickets are being triaged out of 0.3.5. The ones marked "035-roadmap-proposed" may return.

comment:3 Changed 3 months ago by catalyst

Right now on master src/rust/tor_rust/include.am sets CARGO_HOME to $(abs_top_builddir)/src/rust but src/test/test_rust.sh sets it to ${abs_top_builddir:-../../..}/src/rust/.cargo. When building with --enable-cargo-online-mode, I think this means we get two copies of the crate registry and cache: one in src/rust, and one in src/rust/.cargo.

We should avoid this redundancy.

comment:4 Changed 2 months ago by nickm

Keywords: 035-roadmap-proposed added

comment:5 Changed 7 weeks ago by catalyst

I'm leaning toward deleting all explicit settings of CARGO_HOME.

Some old IRC discussion suggests that a difficulty with permissions on Jenkins was involved. Maybe our Jenkins configs don't allow $HOME to be writable by the builder? If so, $HOME/.cargo (the default) wouldn't work. I think that's not a strong reason to clutter other parts of our build system with it; we can adjust the Jenkins to set CARGO_HOME explicitly if needed.

Also, leaving it as the default will let different invocations of cargo share the same cache, which helps with things like using the Travis CI cache (see #26952 and #24629).

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

Replying to catalyst:

I'm leaning toward deleting all explicit settings of CARGO_HOME.

Some old IRC discussion suggests that a difficulty with permissions on Jenkins was involved. Maybe our Jenkins configs don't allow $HOME to be writable by the builder? If so, $HOME/.cargo (the default) wouldn't work. I think that's not a strong reason to clutter other parts of our build system with it; we can adjust the Jenkins to set CARGO_HOME explicitly if needed.

Also, leaving it as the default will let different invocations of cargo share the same cache, which helps with things like using the Travis CI cache (see #26952 and #24629).

That's what I did in #26038 with https://cgit.alxu.ca/tor.git/commit/?h=misc-rust&id=b75f98ee0d0420ab608b7ae7b1cef21e47c7eea3. I'm still not sure what I meant by "not a no-op, fixes out-of-source builds" though. Someone should test if that actually does anything. (I guess I should.)

comment:7 Changed 7 weeks ago by teor

Parent ID: #24629

comment:8 Changed 7 weeks ago by catalyst

Also src/test/test_rust.sh should probably cd ${abs_top_buildir}/src/rust like the src/rust/tor_rust/include.am rules do, for consistency. It's probably not too necessary, though.

comment:9 Changed 7 weeks ago by teor

Status: newneeds_review

These fixes will be in my #24629 branch.

comment:10 Changed 7 weeks ago by teor

Milestone: Tor: unspecifiedTor: 0.3.5.x-final
Version: Tor: 0.3.1.5-alpha

comment:11 Changed 6 weeks ago by asn

Reviewer: catalyst

comment:12 Changed 6 weeks ago by catalyst

Status: needs_reviewmerge_ready

comment:13 Changed 5 weeks ago by teor

Resolution: fixed
Status: merge_readyclosed

Soon to merge in #24629.

comment:14 Changed 5 weeks ago by nickm

Closing these because the parent (#24629) is now merged.

Note: See TracTickets for help on using tickets.