Opened 11 months ago

Closed 7 months ago

#26398 closed defect (wontfix)

feature gate testing C code from Rust for now

Reported by: isis Owned by: isis
Priority: Very High Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.3.3.1-alpha
Severity: Normal Keywords: rust, tor-test, fast-fix, tor-ci
Cc: Actual Points:
Parent ID: #25386 Points: 1
Reviewer: nickm Sponsor: Sponsor8-can

Description

Any tests which call C code from Rust should be feature gated, by default off for now, and then re-enabled when we have a satisfiable fix for the linker issues in #25386.

Child Tickets

Change History (16)

comment:1 Changed 11 months ago by isis

Status: assignedneeds_review

See my bug26400 branch. Note that the tests on it will fail until #26400 is merged.

comment:3 Changed 11 months ago by catalyst

Status: needs_reviewmerge_ready

Thanks! Looks good to me!

comment:4 Changed 11 months ago by nickm

Isis, Catalyst: Do you still think we should merge this?

comment:5 in reply to:  4 ; Changed 11 months ago by teor

Replying to nickm:

Isis, Catalyst: Do you still think we should merge this?

I think I'll need it for the PrivCount noise branch. Or did we fix the linker issues?

comment:6 in reply to:  5 ; Changed 11 months ago by isis

Replying to teor:

Replying to nickm:

Isis, Catalyst: Do you still think we should merge this?

I think I'll need it for the PrivCount noise branch. Or did we fix the linker issues?


Looking at the current status of #25386 it seems there's still going to be linker issues, particularly from doctests, for the time being until everything has rustc 1.27 stable (when the -C option was added to rustdoc).

It might be good to have a configurable way to disable this stuff until all the issues are resolved, that way teor and I can continue writing tests as we would normally for now. In the future, it also gives us an easier way to enable running the tests all the time, when we remove C code (e.g. if I were to write a sha2 or keccak implementation and we used that instead of the C code).

Version 0, edited 11 months ago by isis (next)

comment:7 in reply to:  6 Changed 10 months ago by catalyst

Status: merge_readyneeds_review

Replying to isis:

Looking at the current status of #25386 it seems there's still going to be linker issues, particularly from doctests, for the time being until everything has rustc 1.27 stable (when the -C option was added to rustdoc). But also, feel free to disagree.

It might be good to have a configurable way to disable this stuff until all the issues are resolved, that way teor and I can continue writing tests as we would normally for now. In the future, it also gives us an easier way to enable running the tests all the time, when we remove C code (e.g. if I were to write a sha2 or keccak implementation and we used that instead of the C code).

After thinking about the current situation some more, I think we want to adopt something like this approach. Right now there is a restricted set of Rust testing configurations that we run in Travis, instead of the more comprehensive set that we used to do. Basically, on master, none of the older Travis subjobs with Rust enabled pass tests because of linker problems with --enable-fragile-hardening.

I put this back in "needs_review" because I want to make sure we do something reasonable with the doctests in the crypto crate, and a few potentially conflicting changes to Rust tests have happened on master since the original pull request.

comment:8 Changed 10 months ago by dgoulet

Reviewer: nickm

comment:9 Changed 10 months ago by nickm

I've merged this to master, and added some previously missing items to build.rs.

Before we close this, I think we should have a note about how to actually invoke these tests, if you do want to enable them. I tried adding --features=test-c-from-rust to test_rust.sh, but it makes most of our crates' tests fail.

comment:10 Changed 10 months ago by nickm

Status: needs_reviewneeds_information

comment:11 Changed 10 months ago by nickm

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

(Also I'm inclined to say "no 0.3.4 backport" here.)

comment:12 in reply to:  9 Changed 10 months ago by chelseakomlo

Replying to nickm:

I've merged this to master, and added some previously missing items to build.rs.

Before we close this, I think we should have a note about how to actually invoke these tests, if you do want to enable them. I tried adding --features=test-c-from-rust to test_rust.sh, but it makes most of our crates' tests fail.

Adding a note that fixing https://trac.torproject.org/projects/tor/ticket/25386#comment:74 will allow us to remove this feature gate, and something that we should prioritize.

In the next couple weeks I will organize a discussion at a patch party and work with some Rust people to make sure we can resolve this in a way that allows us to run the full test suite (if someone else wants to take this on organizing this sooner, please do and I will attend said discussion).

comment:13 Changed 9 months ago by chelseakomlo

As an update from https://trac.torproject.org/projects/tor/ticket/25386, it looks like only doctests and compiling with sanitizers are not working as expected.

comment:14 Changed 9 months ago by teor

Parent ID: #25386

comment:15 Changed 8 months ago by nickm

Milestone: Tor: 0.3.5.x-finalTor: unspecified

comment:16 Changed 7 months ago by nickm

Resolution: wontfix
Status: needs_informationclosed

Now that we have alex's fixes, we don't need to make this change.

Note: See TracTickets for help on using tickets.