cargo test fails from crates.io tarball

[ignatenkobrain]

+ /usr/bin/env CARGO_HOME=.cargo RUSTC_BOOTSTRAP=1 /usr/bin/cargo test -j48 -Z avoid-dev-deps --release --no-fail-fast
BUILDSTDERR:        Fresh smallvec v0.6.10
BUILDSTDERR:    Compiling unicode-normalization v0.1.8 (/builddir/build/BUILD/unicode-normalization-0.1.8)
BUILDSTDERR:      Running `/usr/bin/rustc --crate-name unicode_normalization src/lib.rs --color never --emit=dep-info,link -C opt-level=3 --test -C metadata=43229941e7e0e6a1 -C extra-filename=-43229941e7e0e6a1 --out-dir /builddir/build/BUILD/unicode-normalization-0.1.8/target/release/deps -L dependency=/builddir/build/BUILD/unicode-normalization-0.1.8/target/release/deps --extern smallvec=/builddir/build/BUILD/unicode-normalization-0.1.8/target/release/deps/libsmallvec-8bcaab121ba3708c.rlib -Copt-level=3 -Cdebuginfo=2 -Clink-arg=-Wl,-z,relro,-z,now -Ccodegen-units=1`
BUILDSTDERR: error[E0583]: file not found for module `test`
BUILDSTDERR:   --> src/lib.rs:75:5
BUILDSTDERR:    |
BUILDSTDERR: 75 | mod test;
BUILDSTDERR:    |     ^^^^
BUILDSTDERR:    |
BUILDSTDERR:    = help: name the file either test.rs or test/mod.rs inside the directory "src"
BUILDSTDERR: error: aborting due to previous error
BUILDSTDERR: For more information about this error, try `rustc --explain E0583`.
BUILDSTDERR: error: Could not compile `unicode-normalization`.
BUILDSTDERR: Caused by:
BUILDSTDERR:   process didn't exit successfully: `/usr/bin/rustc --crate-name unicode_normalization src/lib.rs --color never --emit=dep-info,link -C opt-level=3 --test -C metadata=43229941e7e0e6a1 -C extra-filename=-43229941e7e0e6a1 --out-dir /builddir/build/BUILD/unicode-normalization-0.1.8/target/release/deps -L dependency=/builddir/build/BUILD/unicode-normalization-0.1.8/target/release/deps --extern smallvec=/builddir/build/BUILD/unicode-normalization-0.1.8/target/release/deps/libsmallvec-8bcaab121ba3708c.rlib -Copt-level=3 -Cdebuginfo=2 -Clink-arg=-Wl,-z,relro,-z,now -Ccodegen-units=1` (exit code: 1)

I would appreciate if you would just make it under some cfg or something like that so that it does not fail for us.

[kentfredric]

I guess this is slightly ironic.

The Cargo.toml explicitly strips some tests, and its the stripping of these tests that causes cargo test to fail.

Please endeavour to have some sort of useful tests in the shipped crate. Its important for linux-vendor QA purposes ( pretend linux vendors are a kind of upstream who have to make sure everything works on a new rust the same way you would, but have to use a shipped crate as a baseline )

[Manishearth]

Adding tests to this crate bloats the size of the published crate (which matters for many consumers), they were explicitly stripped because the tests rely on unicode tables.

[kentfredric]

That's fine, just I find it funny, as the problem emerges from there being an incomplete exorcising of tests :D

Though it would be nice if there was some medium ground, where there's enough test in place to provide something useful to end users, and having more exhaustive tests gated behind something so as not to cause that kind of pain.

[nox]

Both is_combining_mark and UnicodeNormalization are public items of this crate, and they are the only two things used by the test module.

As far as I can tell, this module and the stuff from normalization_tests can be moved to an integration test in tests/ to be properly excluded from the published package, letting downstream vendors run the other tests successfully.

[Manishearth]

Totally okay with merging a PR for that, though really without the NormalizationTests tests there's not much of a point to running cargo test.

…

On Mon, Oct 28, 2019, 5:27 AM Anthony Ramine ***@***.***> wrote: Both is_combining_mark and UnicodeNormalization are public items of this crate, and they are the only two things used by the test module. As far as I can tell, this module and the stuff from normalization_tests can be moved to an integration test in tests/ to be properly excluded from the published package, letting downstream vendors run the other tests successfully. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#38?email_source=notifications&email_token=AAMK6SA4INJZGAAHUOU266LQQ3LB5A5CNFSM4HZRFJL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECMWJKQ#issuecomment-546923690>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMK6SCN3FVVJOZHQB5ZHEDQQ3LB5ANCNFSM4HZRFJLQ> .

[kentfredric]

Yeah, turns out to be a little more complicated than nox thought.

I've managed so far to move a bunch of the tests to tests/, in a way that would make it more easy to nuke the ones that were "heavy" without consequence.

Just a few stragglers remain that annoyingly require access to private interfaces to function.

Got a few potential ways of making this work though, one idea is a build.rs that probes for the existence of the normalization_tests.rs file, and passes a flag to enable the relevant tests.

That aught to work transparently for both authors and consumers.

[Manishearth]

Using a build.rs that way seems quite brittle :/ -Manish Goregaokar

…

On Mon, Oct 28, 2019 at 8:41 AM Kent Fredric ***@***.***> wrote: Yeah, turns out to be a little more complicated than nox thought. I've managed so far to move a bunch of the tests to tests/, in a way that would make it more easy to nuke the ones that were "heavy" without consequence. Just a few stragglers remain that annoyingly require access to private interfaces to function. Got a few potential ways of making this work though, one idea is a build.rs that probes for the existence of the normalization_tests.rs file, and passes a flag to enable the relevant tests. That aught to work transparently for both authors and consumers. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#38?email_source=notifications&email_token=AAMK6SEVDBU2B2VNG3KAKCTQQ4B3FA5CNFSM4HZRFJL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECNKXHQ#issuecomment-547007390>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMK6SB6VH3DFFUYIQF4TEDQQ4B3FANCNFSM4HZRFJLQ> .

[kentfredric]

well... you don't have to use build.rs, just means you'll need RUST_FLAGS="--cfg bloaty_test" cargo test to make those tests spark up ;)

[Manishearth]

Given that these tests really really should be run by default that solution is not acceptable either.

[Manishearth]

Ultimately, "we want to run cargo test from packaging" is not a use case we particularly care about, I'm fine supporting it but it should not make things worse for the default use case. Those tests are the important ones, the other tests don't really matter much. If you're excluding those I don't even see the point of running cargo test.

It's a pretty common exercise to exclude test files from the published package, fwiw. Some of the regex dependencies do this as well since they include large text corpuses.

[kentfredric]

not a use case we particularly care about

That translates to "Do not care about linux vendors at all", which translates to limiting your own adoption ( and seriously limits the quality of rust at large, as there's plenty of crates that do fail tests for reasons that are real problems, and we can expect to see this get worse when features get deprecated )

Some of this misery is worse than it aught to be because there's no good way to have tests outside src/ which share the visibility of src/

Abusing #[path = "../../foo.rs"] in the right places kinda works, but its also probably gonna get me yelled at.

If tests could be entirely in tests/, then you could employ the exclude rules with complete immunity, nothing bad could happen because you can safely nuke anything outside src/ without risking it breaking src/

[kentfredric]

Something that would be gross, but survivable, is repeat the PR, but with the flag inverted. That way at least, people who want to run tests downstream only have to dance with setting RUSTFLAGS=, not with having to patch the entire crate. ( But it would be by default, still horribly broken )

[Manishearth]

That translates to "Do not care about linux vendors at all", which translates to limiting your own adoption

You haven't sufficiently made the case for why you need to run tests here. You're effectively asking for cargo test for this crate to work but not run any important tests, which seems very illogical to me: why run cargo test at all? You're not getting anything out of it.

Similarly, why not run ./scripts/unicode.py followed by cargo test?

---

If you can figure out a way to stick the normalization tests in tests/ that would be fine.

[Manishearth]

But it would be by default, still horribly broken

This comes off as super arrogant given that your original solution was to make this crate similarly horribly broken for developers. The normalization tests are the important tests, if they're not run by default there's not much point to having them. We're not going to flag off the main tests in this crate to support your use case. That's ridiculous.