Remove tests from package that is published #340
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #340 +/- ##
=======================================
Coverage 97.76% 97.76%
=======================================
Files 20 20
Lines 4343 4343
=======================================
Hits 4246 4246
Misses 97 97 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
As I understand it some packagers like to have the tests be part of the package because they can be used to verify that it works. Honestly I'm not convinced of the actual supply chain benefit of this change -- are you going to vet every line of code in this package for every update we release? If not, you're already trusting us. (Dealt with the cargo-deny failure in #341.) |
That's basically what we are currently doing. (Well we just review the diffs, not the full code, but that comes down to the same thing if you initially reviewed "everything") |
|
Hi and thanks for your quick reply. At the moment, whenever we update our dependencies, we are actually going over all the changes between the old and the new version (for this specific crate, the changes would e.g. look like this https://diff.rs/rustls-webpki/0.102.8/0.103.1). That includes dependencies of other dependencies as well (yes it's a long list). In these situations we are mainly looking for things like changed build scripts, added binary files or data in code, suspicious looking code additions, changed ownership situations etc. We are not able to review each individual line in the context of the crate to assure that the functionality hasn't been tempered with deep down somewhere, but yes we are looking at every changed line in every changed file to check if something malicious is going on. In this way we hope to improve the supply chain safety for our project and the safety of the Rust ecosystem in general as well. Edit: Oh sorry, weiznich was quicker ... 😅 |
There was a problem hiding this comment.
FWIW we did this in rustls/rustls#2077 albeit for different reasons. In this change we go from 801KB to 77.6KB for the .crate size, which I think is worth doing irrespective of anything else.
I had understood that .crate files were never guaranteed to contain tests in a runnable form, so I can't really see that as being a good upstream for packaging?
|
Size wise if I simply execute |
|
Okay, that seems like a nice win -- let's do it. Thanks! |
The tests include some scripts and binary files that can be hard to review when caring about supply chain safety.
Head branch was pushed to by a user without write access
|
Sorry I put an accidental merge commit in my branch there. Now everything should be sorted again. Thank you both for responding to this PR so quickly! 🙏 |
Hi everyone 👋
While reviewing dependency updates in our project (trying to assure supply chain safety) I noticed that the
testsdirectory contains many binary files and a couple of scripts as well. That makes it harder to review when checking the supply chain and I was wondering if it would be possible to remove this directory from the published package. That would remove a potential vector for a security vulnerability in the future.The downside of course would be that the tests couldn't be run from the crate package anymore, but I'm not sure how popular that is.
Best regards!