search

app-dirs-rs / app_dirs2

A maintained fork of the app-dirs-rs project.
MIT License
35 stars 7 forks source link

bump test-case from v2 to v3, exclude some files from published crates #37

Closed decathorpe closed 9 months ago

decathorpe commented 9 months ago

The update from test-case v2 to v3 does not require code changes in this crate.

I also added a package.exclude setting to Cargo.toml to prevent some development / CI files that are not useful for people who just use this crate as a dependency from being included in crates that are published in crates.io, which makes the download size a bit smaller (18.3 kB → 17.5 kB).

kornelski commented 9 months ago

Thanks!

Maybe the list should be include? There's probably fewer files to add than to exclude.

decathorpe commented 9 months ago

If I'm counting correctly, the list of includes would be longer: include = ["/Cargo.toml", "/README.md", "LICENSE.txt", "/examples", "/src", "/tests"] is six elements long, the excludes I added are five elements long ;)

kornelski commented 9 months ago

crates.io packages don't use examples or tests, but it's fine either way.

decathorpe commented 9 months ago

Thanks for merging!

PS: It's not quite true that crates published on crates.io don't need examples or tests. Linux distros use the crates as published on crates.io, and if those don't include files for examples or integration tests, running cargo test for those crates can be tedious or impossible.

kornelski commented 9 months ago

There is nothing in Cargo nor crates.io that uses these files. Cargo's compile test for packaging doesn't check if they even compile. This is an unsupported workflow, and such packaged files are untested.

I think distros that try to use these files rely on an undocumented side effect of lax defaults in Cargo, and that is not a good idea. It's using Cargo's packages in a way that Cargo didn't mean to support, and judges correctness of packages by files that weren't supposed to be used.

Published packages are taken out of a workspace and have their Cargo.toml rewritten, so running tests on them doesn't match behavior in local development, nor CI/CD (e.g. shared fixtures won't work).

Plus this creates a tension between keeping crates.io packages for Cargo lean, and having many tests and examples, especially when additional fixtures and assets are needed. That's unfortunate, because this way crate authors are forced to choose the tradeoff, and can't make the best package for both even if they wanted.

There are cases where it's even tricky to just exclude tests, e.g. tests may need to be inline in src/ to access private APIs, but still need external fixtures.

I'm afraid that this approach creates busywork for everyone involved, and I explicitly don't support it in any of my crates.

MarijnS95 commented 9 months ago

If I'm counting correctly, the list of includes would be longer: include = ["/Cargo.toml", "/README.md", "LICENSE.txt", "/examples", "/src", "/tests"] is six elements long, the excludes I added are five elements long ;)

Cargo.toml is always included: https://doc.rust-lang.org/cargo/reference/manifest.html#the-exclude-and-include-fields. LICENSE.txt could also be included automatically if license-file is set (but it doesn't seem to be encouraged when license is already set).