Link Rust Tests to C Dependencies in Tor (allow integration testing from Rust to C)

currently, it is not possible to call C Tor, directly or indirectly, from rust tests. one of the following must be done:

1. provide rust stubs for all C functions that may be needed for tests (impractical)

2. test rust functions from C (so we will have C tests calling Rust functions calling C functions)

3. link C functions into rust doctests (preferred)

4. never call C-using rust functions in tests (leads to poor test coverage, very bad)

my branch https://cgit.alxu.ca/tor.git/commit/?h=fix-rust-tests implements option 3 poorly. this is a bad solution firstly because it is very ugly, and secondly because it does not properly pass the system linking arguments, e.g. -L/opt/ssl. thirdly, it may hide problems in or cause to be compiled incorrectly dependency crates.

this ticket blocks a number of rust improvements, since of course we would like to actually test the improvements, and doctests are the best way to do it in rust.

changed milestone to %Tor: unspecified

added 033-unreached-backport 034-included-20180401 034-triage-20180328 035-removed-20180711 component::core tor/tor milestone::Tor: unspecified points::3 priority::high review-group-34 reviewer::isis rust severity::normal sponsor::Q status::new tor-test type::defect version::tor 0.3.3.1-alpha labels

Trac:
Type: enhancement to defect
Points: N/A to 1
Version: N/A to Tor: 0.3.2.1-alpha
Milestone: N/A to Tor: 0.3.4.x-final
Component: - Select a component to Core Tor/Tor
Keywords: N/A deleted, rust, tor-test added

Hello71 has a branch here: https://cgit.alxu.ca/tor.git/commit/?h=fix-rust-tests

But it needs to inherit libraries and library flags from the Makefile, And it needs to build Rust with asan, if asan is turned on.

We want to pass the -Z sanitizer flag to Rust: https://github.com/japaric/rust-san

Trac:
Status: new to needs_revision

still doesn't work with --enable-fragile-hardening due to ubsan, might work if that's commented out (or maybe not?)

I still think the best way is to build a "libtor.so" and link all the tests with it, so we don't have to pass all the dependencies in or interrogate the toolchain about how to link sanitizers.

Trac:
Status: needs_revision to needs_review

Let's get a minimal solution working first, and then look at ubsan and shared libraries.

Is the set of ".a" files minimal? Or should we use an environmental variable with that set of files instead? (For example, the set of tor source files linked into src/test/test would be a good place to start.)

Trac:
Keywords: N/A deleted, 033-backport added
Version: Tor: 0.3.2.1-alpha to Tor: 0.3.3.1-alpha

Trac:
Keywords: N/A deleted, review-group-34 added

Reviewer for week 03/19

Trac:
Reviewer: N/A to isis

Hello Hello71! (hah) Thanks a whole bunch for looking into this and for the patches! This looks much better than the attempts I was making going through cargo and rust #[cfg] linker directives.

Some notes on a couple issues:

1. I reverted 350f76d3 and 8748ddac in my bug25386 branch, as they both affected the TravisCI builds to enable OSX (which takes forever, hence why it's disabled by default) and also to (mostly?) disable clang (not sure why this was done? We like testing our C with different compilers, and clang's LeakSanitizer has caught leaks where GCC has not). Generally, in your personal repo, if you'd like to enable OSX builds for a branch, the way to do so is to make another branch off of it with the commits to enable the OSX builds, so that they don't accidentally get merged and make everyone's builds suddenly slow. (Also the changes seem to have resulted in building with GCC and --enable-cargo-online-mode twice somehow?)

2. Nick's make test-rust target from #25071 (moved) has been broken, due to no longer finding the test_rust.sh script.

3. make check fails. (That Travis build is just a copy of your branch that I prefixed with testing/hello71/ to keep it out of my branch namespace.) It looks like it's pretty angry about missing symbols. Locally, on my machine, building out-of-tree, it's mad about: {{{ FAIL: src/test/test_rust.sh

   error: could not find Cargo.toml in /home/isis/code/torproject/tor-build/src/rust or any parent directory}}} (Mytor/directory is at/home/isis/code/torproject/tor`.) This will [https://jenkins.torproject.org/ break our Jenkins builds].

I think once we're able to get these things fixed, then we can start on the UBSan/other sanitiser issues (possibly merging in-between).

Replying to isis:

Hello Hello71! (hah) Thanks a whole bunch for looking into this and for the patches! This looks much better than the attempts I was making going through cargo and rust #[cfg] linker directives.

Some notes on a couple issues:

1. I reverted 350f76d3 and 8748ddac in my bug25386 branch, as they both affected the TravisCI builds to enable OSX (which takes forever, hence why it's disabled by default) and also to (mostly?) disable clang (not sure why this was done? We like testing our C with different compilers, and clang's LeakSanitizer has caught leaks where GCC has not). Generally, in your personal repo, if you'd like to enable OSX builds for a branch, the way to do so is to make another branch off of it with the commits to enable the OSX builds, so that they don't accidentally get merged and make everyone's builds suddenly slow. (Also the changes seem to have resulted in building with GCC and --enable-cargo-online-mode twice somehow?)

2. Nick's make test-rust target from #25071 (moved) has been broken, due to no longer finding the test_rust.sh script.

3. make check fails. (That Travis build is just a copy of your branch that I prefixed with testing/hello71/ to keep it out of my branch namespace.) It looks like it's pretty angry about missing symbols. Locally, on my machine, building out-of-tree, it's mad about: {{{ FAIL: src/test/test_rust.sh

   error: could not find Cargo.toml in /home/isis/code/torproject/tor-build/src/rust or any parent directory}}} (Mytor/directory is at/home/isis/code/torproject/tor`.) This will break our Jenkins builds.

I think once we're able to get these things fixed, then we can start on the UBSan/other sanitiser issues (possibly merging in-between).

Oh, one more note:

4. This approach seems pretty sane, but one thing it won't allow is doing: {{{#!sh cd src/rust/protover cargo test }}} and getting a positive result. I'm not sure how much we should care about that, especially if—as a team—we're going to eventually agree that things will simply be rewritten in Rust, as opposed to maintained in both languages. (I.e. it's only a problem while we maintain both, and maintain the FFI boundary for both.)

Replying to isis:

1. I reverted 350f76d3 and 8748ddac in my bug25386 branch, as they both affected the TravisCI builds to enable OSX (which takes forever, hence why it's disabled by default) and also to (mostly?) disable clang (not sure why this was done? We like testing our C with different compilers, and clang's LeakSanitizer has caught leaks where GCC has not). Generally, in your personal repo, if you'd like to enable OSX builds for a branch, the way to do so is to make another branch off of it with the commits to enable the OSX builds, so that they don't accidentally get merged and make everyone's builds suddenly slow. (Also the changes seem to have resulted in building with GCC and --enable-cargo-online-mode twice somehow?)

This is why I don't like Travis's method of putting the configuration inside the repository. Here, I specifically want the changes to only apply to this branch. I suppose I could move that to another branch, and base this one on that one, but that sounds like a huge pain.

2. Nick's make test-rust target from #25071 (moved) has been broken, due to no longer finding the test_rust.sh script.

I will investigate.

3. make check fails. (That Travis build is just a copy of your branch that I prefixed with testing/hello71/ to keep it out of my branch namespace.) It looks like it's pretty angry about missing symbols.

Yes, I am aware, it doesn't work right with sanitizers. I discussed that with teor to some extent, but as I recall, I said "we should make a 'libtor.so'", he said "no, that will cause more problems", and I thought "I don't see any better way of doing it. I'll come back to it later." and then forgot to come back to it. I think there might be a solution in rustc -Csomething-or-other though.

Locally, on my machine, building out-of-tree, it's mad about: {{{ FAIL: src/test/test_rust.sh ===========================

error: could not find `Cargo.toml` in `/home/isis/code/torproject/tor-build/src/rust` or any parent directory`
}}}
(My `tor/` directory is at `/home/isis/code/torproject/tor`.) This will [break our Jenkins builds](https://jenkins.torproject.org/).

I think once we're able to get these things fixed, then we can start on the UBSan/other sanitiser issues (possibly merging in-between).

Ah, quite possible, I did not test this. I will investigate.

Replying to isis:

Oh, one more note:

4. This approach seems pretty sane, but one thing it won't allow is doing: {{{#!sh cd src/rust/protover cargo test }}} and getting a positive result. I'm not sure how much we should care about that, especially if—as a team—we're going to eventually agree that things will simply be rewritten in Rust, as opposed to maintained in both languages. (I.e. it's only a problem while we maintain both, and maintain the FFI boundary for both.)

This seems easy to rectify, just make test_rust.sh take a crate to test, or potentially check the current directory or something. In theory, we could do it in cargo too, but I don't know how to add libraries only during testing (or if such a thing exists).

Trac:
Status: needs_review to assigned
Owner: N/A to Hello71

Trac:
Status: assigned to needs_revision

Trac:
Sponsor: N/A to Sponsor8-can
Points: 1 to 3

Trac:
Cc: teor, isis to teor, isis, chelseakomlo

Hi, apologies for the late review, but is there a reason why the #3 (closed) option in the description is for doctests only? Ideally, we would want linking for at minimum integration tests, ideally all levels of tests.

Replying to chelseakomlo:

Hi, apologies for the late review, but is there a reason why the #3 (closed) option in the description is for doctests only? Ideally, we would want linking for at minimum integration tests, ideally all levels of tests.

er, I meant all tests. everything that gets run by cargo test.

Cool, thanks for clarifying and for working on this.

Another necessary criteria to think about for this effort is being able to run individual tests, or tests at a module level to ensure a quick feedback loop.

Fixed all problems except sanitizers not working. Asked #rust-beginners about building it using cargo, concluded it's too much work, and besides, scripts/cargo_test doesn't seem that much harder than cargo test. It would be nice to not duplicate the list of libraries, but I think that's a discussion for another time. this seems Good Enough for now.

Trac:
Status: needs_revision to needs_review
Version: Tor: 0.3.3.1-alpha to Tor: 0.3.3.2-alpha