Skip to content
Permalink
Browse files
Don't try to link C from rust doctests for nss detection
This is really annoying, since we can't use cfg(test) for doctests.
  • Loading branch information
nmathewson committed Sep 16, 2018
1 parent 078debb commit a8ac21fbb5f22b68ffa019b575085c142293bc40
@@ -23,3 +23,9 @@ default = []
test-c-from-rust = [
"crypto/test-c-from-rust",
]

# We have to define a feature here because doctests don't get cfg(test),
# and we need to disable some C dependencies when running the doctests
# because of the various linker issues. See
# https://github.com/rust-lang/rust/issues/45599
test_linking_hack = []
@@ -30,3 +30,9 @@ rand_core = { version = "=0.2.0-pre.0", default-features = false }
# execute with `cargo test`. Due to numerous linker issues (#25386), this is
# currently disabled by default.
test-c-from-rust = []

# We have to define a feature here because doctests don't get cfg(test),
# and we need to disable some C dependencies when running the doctests
# because of the various linker issues. See
# https://github.com/rust-lang/rust/issues/45599
test_linking_hack = []
@@ -14,3 +14,9 @@ name = "external"
path = "lib.rs"
crate_type = ["rlib", "staticlib"]

[features]
# We have to define a feature here because doctests don't get cfg(test),
# and we need to disable some C dependencies when running the doctests
# because of the various linker issues. See
# https://github.com/rust-lang/rust/issues/45599
test_linking_hack = []
@@ -4,6 +4,11 @@ version = "0.0.1"
name = "protover"

[features]
# We have to define a feature here because doctests don't get cfg(test),
# and we need to disable some C dependencies when running the doctests
# because of the various linker issues. See
# https://github.com/rust-lang/rust/issues/45599
test_linking_hack = []

[dependencies]
libc = "=0.2.39"
@@ -27,4 +32,3 @@ path = "../tor_log"
name = "protover"
path = "lib.rs"
crate_type = ["rlib", "staticlib"]

@@ -10,7 +10,6 @@ use std::str::FromStr;
use std::string::String;

use external::c_tor_version_as_new_as;
use external::c_tor_is_using_nss;

use errors::ProtoverError;
use protoset::ProtoSet;
@@ -125,6 +124,17 @@ impl From<Protocol> for UnknownProtocol {
}
}

#[cfg(feature="test_linking_hack")]
fn have_linkauth_v1() -> bool {
true
}

#[cfg(not(feature="test_linking_hack"))]
fn have_linkauth_v1() -> bool {
use external::c_tor_is_using_nss;
! c_tor_is_using_nss()
}

/// Get a CStr representation of current supported protocols, for
/// passing to C, or for converting to a `&str` for Rust.
///
@@ -142,7 +152,7 @@ impl From<Protocol> for UnknownProtocol {
///
// C_RUST_COUPLED: protover.c `protover_get_supported_protocols`
pub(crate) fn get_supported_protocols_cstr() -> &'static CStr {
if c_tor_is_using_nss() {
if ! have_linkauth_v1() {
cstr!("Cons=1-2 \
Desc=1-2 \
DirCache=1-2 \
@@ -11,3 +11,9 @@ name = "smartlist"
path = "lib.rs"
crate_type = ["rlib", "staticlib"]

[features]
# We have to define a feature here because doctests don't get cfg(test),
# and we need to disable some C dependencies when running the doctests
# because of the various linker issues. See
# https://github.com/rust-lang/rust/issues/45599
test_linking_hack = []
@@ -11,3 +11,9 @@ name = "tor_allocate"
path = "lib.rs"
crate_type = ["rlib", "staticlib"]

[features]
# We have to define a feature here because doctests don't get cfg(test),
# and we need to disable some C dependencies when running the doctests
# because of the various linker issues. See
# https://github.com/rust-lang/rust/issues/45599
test_linking_hack = []
@@ -9,6 +9,11 @@ path = "lib.rs"
crate_type = ["rlib", "staticlib"]

[features]
# We have to define a feature here because doctests don't get cfg(test),
# and we need to disable some C dependencies when running the doctests
# because of the various linker issues. See
# https://github.com/rust-lang/rust/issues/45599
test_linking_hack = []

[dependencies]
libc = "0.2.39"
@@ -14,3 +14,9 @@ path = "../tor_util"
[dependencies.protover]
path = "../protover"

[features]
# We have to define a feature here because doctests don't get cfg(test),
# and we need to disable some C dependencies when running the doctests
# because of the various linker issues. See
# https://github.com/rust-lang/rust/issues/45599
test_linking_hack = []
@@ -17,3 +17,9 @@ path = "../tor_log"
[dependencies]
libc = "=0.2.39"

[features]
# We have to define a feature here because doctests don't get cfg(test),
# and we need to disable some C dependencies when running the doctests
# because of the various linker issues. See
# https://github.com/rust-lang/rust/issues/45599
test_linking_hack = []
@@ -10,6 +10,7 @@ for cargo_toml_dir in "${abs_top_srcdir:-../../..}"/src/rust/*; do
cd "${abs_top_builddir:-../../..}/src/rust" && \
CARGO_TARGET_DIR="${abs_top_builddir:-../../..}/src/rust/target" \
"${CARGO:-cargo}" test ${CARGO_ONLINE-"--frozen"} \
--features "test_linking_hack" \
${EXTRA_CARGO_OPTIONS} \
--manifest-path "${cargo_toml_dir}/Cargo.toml" || exitcode=1
fi

1 comment on commit a8ac21f

@alexcrichton

This comment has been minimized.

Copy link
Contributor

@alexcrichton alexcrichton commented on a8ac21f Oct 2, 2018

I did some investigation here because I couldn't quite figure out why this was necessary, and it seemed suspicious!

At least on the current master branch (may not have been true at the time though!) I think doctests are a red herring. The problem here was that the protover crate links to the external crate which requires C symbols but doesn't link to them. (or rather it doesn't emit directives necessary to link those symbols in with the Rust compiler).

The protover crate then has two test suites. One is the library itself compiled with --test, and the other is the tests/protover.rs file. Both are compiled with --test, but the tests/protover.rs integration test links against an actual copy of libprotover.rlib, which is itself not compiled with --test. That's why I think you needed the extra feature here, you want both tests to not link to external, but only one is compiled with --cfg test.

(there may have also been an issue with doctests too! I'd have to look further into the failures)

FWIW I think the best long term fix for this is to update the external crate to use ../build.rs like the crypto crate does and have it print the various directives necessary to link in the C symbols it needs. Once that's done I believe this commit/feature hack can be removed (yay!)

Please sign in to comment.