Devolutions / picky-rs

Picky portable PKI implementation and microservice
Apache License 2.0
52 stars 27 forks source link

picky-asn1 0.9 release is missing dev-dependencies #324

Open elmarco opened 3 days ago

elmarco commented 3 days ago

For some reason, it's missing picky-asn1-der, and failing to build doctest. Maybe you need workspace for that to work?

   Doc-tests picky_asn1
     Running `/usr/bin/rustdoc --edition=2021 --crate-type lib --color auto --crate-name picky_asn1 --test src/lib.rs --test-run-directory /builddir/build/BUILD/rust-picky-asn1-0.9.0-build/picky-asn1-0.9.0 -L dependency=/builddir/build/BUILD/rust-picky-asn1-0.9.0-build/picky-asn1-0.9.0/target/rpm/deps -L dependency=/builddir/build/BUILD/rust-picky-asn1-0.9.0-build/picky-asn1-0.9.0/target/rpm/deps --extern oid=/builddir/build/BUILD/rust-picky-asn1-0.9.0-build/picky-asn1-0.9.0/target/rpm/deps/liboid-ebff3ae5c9007caf.rlib --extern picky_asn1=/builddir/build/BUILD/rust-picky-asn1-0.9.0-build/picky-asn1-0.9.0/target/rpm/deps/libpicky_asn1-82c7b3838a677f6c.rlib --extern serde=/builddir/build/BUILD/rust-picky-asn1-0.9.0-build/picky-asn1-0.9.0/target/rpm/deps/libserde-ad2146265121d7c5.rlib --extern serde_bytes=/builddir/build/BUILD/rust-picky-asn1-0.9.0-build/picky-asn1-0.9.0/target/rpm/deps/libserde_bytes-1ddbdc6dc57c90ba.rlib -C embed-bitcode=no --check-cfg 'cfg(docsrs)' --check-cfg 'cfg(feature, values("chrono", "chrono_conversion", "time", "time_conversion", "zeroize"))' --error-format human`

running 13 tests
test src/wrapper.rs - wrapper::HeaderOnly (line 452) ... FAILED
test src/tag.rs - tag::TagPeeker (line 201) ... FAILED
test src/wrapper.rs - wrapper::BitStringAsn1Container (line 555) ... FAILED
test src/wrapper.rs - wrapper::OctetStringAsn1Container (line 607) ... FAILED
test src/wrapper.rs - wrapper::Optional (line 657) ... FAILED
test src/bit_string.rs - bit_string::BitString::is_set (line 145) ... ok
test src/bit_string.rs - bit_string::BitString (line 21) ... ok
test src/bit_string.rs - bit_string::Vec<u8>::from (line 250) ... ok
test src/bit_string.rs - bit_string::BitString::with_bytes_and_len (line 63) ... ok
test src/bit_string.rs - bit_string::BitString::payload_view_mut (line 222) ... ok
test src/bit_string.rs - bit_string::BitString::set_num_bits (line 116) ... ok
test src/bit_string.rs - bit_string::BitString::with_bytes (line 90) ... ok
test src/bit_string.rs - bit_string::BitString::payload_view (line 205) ... ok

failures:

---- src/wrapper.rs - wrapper::HeaderOnly (line 452) stdout ----
error[E0433]: failed to resolve: use of undeclared crate or module `picky_asn1_der`
  --> src/wrapper.rs:460:15
   |
11 | let encoded = picky_asn1_der::to_vec(&tag_only).expect("couldn't serialize");
   |               ^^^^^^^^^^^^^^ use of undeclared crate or module `picky_asn1_der`
   |
help: there is a crate or module with a similar name
   |
11 | let encoded = picky_asn1::to_vec(&tag_only).expect("couldn't serialize");
   |               ~~~~~~~~~~

error[E0433]: failed to resolve: use of undeclared crate or module `picky_asn1_der`
  --> src/wrapper.rs:466:52
   |
17 | let decoded: HeaderOnly<ExplicitContextTag0<()>> = picky_asn1_der::from_bytes(&buffer).expect("couldn't deserialize");
   |                                                    ^^^^^^^^^^^^^^ use of undeclared crate or module `picky_asn1_der`
   |
help: there is a crate or module with a similar name
   |
17 | let decoded: HeaderOnly<ExplicitContextTag0<()>> = picky_asn1::from_bytes(&buffer).expect("couldn't deserialize");
   |                                                    ~~~~~~~~~~

error: aborting due to 2 previous errors
CBenoit commented 1 day ago

Indeed, the dev-dependency is specified but with no version: https://github.com/Devolutions/picky-rs/blob/f0105574876eb91040e3587c93c98fbce751ff92/picky-asn1/Cargo.toml#L27

It’s typically fine since we just run the tests from the workspace, and the crate is then found. If you downloaded the code from crates.io and tried to run the tests, this crate will not be found along. In fact, as you noted, the dev-dependency is automatically stripped out: https://docs.rs/crate/picky-asn1/0.9.0/source/Cargo.toml#27

I never thought about running the tests outside of the actual development process, and that’s a pretty typical setup so cargo publish does allow that. Do you think it would be worth to allow running the tests from the final package in standalone?

EDIT: As mentioned here, I doubt this is something you should expect to work in general. In this case, we could specify a version number to the local dependency and get it to fallback on crates.io, but testing assets are typically not packaged to save registry space, bandwidth, etc, so in many cases you just can’t expect the tests to build/run outside of the repository. Having dev-dependencies on crates not available on crates.io is also something you may come across.

elmarco commented 22 hours ago

It's always worth to be able to run the tests from released/distributed version.

Fwiw, it seems "path" [dev-dependencies] without a version are stripped. But if you specify the version, then the substitution for crates.io happen. (similar to [dependencies])

CBenoit commented 21 hours ago

It's always worth to be able to run the tests from released/distributed version.

Fair enough! IIRC, crater is also doing this.

Fwiw, it seems "path" [dev-dependencies] without a version are stripped. But if you specify the version, then the substitution for crates.io happen. (similar to [dependencies])

Yes, what is stripped is just the "path" key, which may result in "nothing" when that’s the only thing specified. I said "fallback" because the version published on crates.io is not necessarily the same as the WIP version currently used in the repository.

This is unlikely to be problem in practice for picky-asn1, so I’m okay with going ahead and add the version number, it’s really not much.

I’m not so sure for tests which contain many test assets. http://crates.io will be storing the data forever, and every user is going to have to download it all. I'd rather keep crate files as small as possible. We could likely find a compromise: move the tests requiring assets in the tests/ folder that we don’t publish, and keep the self-contained tests as unit tests runnable using the distributed package alone. Also, if the assets are reasonably small, we may consider embedding them or inlining the contents along the tests instead. The only file that is really big that I remember of is mkcert_all_root_ca_2019_10.txt. Let’s discuss about that in the other issue: https://github.com/Devolutions/picky-rs/issues/325