-
Notifications
You must be signed in to change notification settings - Fork 324
Exclude development scripts from published package #810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Cargo.toml
Outdated
| readme = "README.md" | ||
| keywords = ["buffers", "zero-copy", "io"] | ||
| categories = ["network-programming", "data-structures"] | ||
| include = ["CHANGELOG.md", "LICENSE", "README.md", "SECURITY.md", "Cargo.toml", "src/**/*.rs"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether tests/**/*.rs and benches/**/*.rs should be added too.
They are not needed for the build as a (transitive) dependency of a project but still they are part of the source distribution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are arguments for excluding and including these directories:
For excluding: Almost 100% of the users that consume the code from crates.io don't run or use these tests or benchmarks at all.
For including: There are specific usage patterns which might benefit from having these folders there:
- Linux distributions running tests as part of their package builds
- Possibly crater, but crater also fetches github repositories so that might not be that important
I personally tend to exclude these folders whenever possible as the large majority of users won't need them. For the exceptions there are other solutions. That's also a something shared by other large projects like reqwest or rustls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to include the tests but not benchmarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed an update to do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Including the clippy config file also seems harmless, and might help for anyone who vendor their dependencies using versions from crates.io.
3393061 to
c1d1827
Compare
During a dependency review we noticed that the bytes crate includes various development scripts. These development scripts shouldn't be there as they might, at some point become problematic. As of now they prevent any downstream user from enabling the `[bans.build.interpreted]` option of cargo deny. I opted for using an explicit include list instead of an exclude list to prevent these files from being included in the published packages to make sure that everything that's included is an conscious choice.
c1d1827 to
04952eb
Compare
Darksonn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
During a dependency review we noticed that the bytes crate includes various development scripts. These development scripts shouldn't be there as they might, at some point become problematic. As of now they prevent any downstream user from enabling the
[bans.build.interpreted]option of cargo deny.I opted for using an explicit include list instead of an exclude list to prevent these files from being included in the published packages to make sure that everything that's included is an conscious choice.