Skip to content
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

Exclude more files from the published crate #37

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

musicinmybrain
Copy link
Contributor

The tests were already excluded from the published crate. This change adds Makefile, rustfmt.toml, and the contents of benchmark-builds/, all of which are not used by cargo to build the published crate.

This was suggested in downstream packaging review.

@AlexTMjugador
Copy link
Member

Thank you for the PR, this is a welcome improvement! I'm curious to know whether you have found out about any additional unnecessary files with the help of the cargo diet tool?

@musicinmybrain
Copy link
Contributor Author

Thank you for the PR, this is a welcome improvement! I'm curious to know whether you have found out about any additional unnecessary files with the help of the cargo diet tool?

No, but I just tried building and running it on the branch corresponding to this PR, and it said:

$ cargo diet 
Your crate is perfectly lean!
Nothing changed.

If you really want to prune out everything unnecessary, based on

$ cargo package --list
.cargo_vcs_info.json
.github/renovate.json
.github/workflows/ci.yml
.gitignore
CONTRIBUTORS
COPYING
Cargo.lock
Cargo.toml
Cargo.toml.orig
README.md
src/blocksplitter.rs
[…]
src/zlib.rs

you could probably exclude .gitignore and the contents of .github/. I’m not sure if you view those as worthwhile or not, but I can add them to the PR if you like.

@AlexTMjugador
Copy link
Member

Thanks for sharing the results of running cargo diet! I don't think the .gitignore file and the contents of the .github directory are significant sources of bloat, but since we're already on the topic of getting rid of unnecessary files, I think it would be great to add those to the exclusion list. Could you please do that?

After that, this PR should be good to merge. 🚀

The tests were already excluded from the published crate. This change
adds `Makefile`, `rustfmt.toml`, `.gitignore`, and the contents of
`benchmark-builds/` and `.github/`, all of which are not used by cargo
to build the published crate.
@musicinmybrain
Copy link
Contributor Author

Thanks for sharing the results of running cargo diet! I don't think the .gitignore file and the contents of the .github directory are significant sources of bloat, but since we're already on the topic of getting rid of unnecessary files, I think it would be great to add those to the exclusion list. Could you please do that?

Ok, done!

$ cargo package --list
.cargo_vcs_info.json
CONTRIBUTORS
COPYING
Cargo.lock
Cargo.toml
Cargo.toml.orig
README.md
src/blocksplitter.rs
[…]
src/zlib.rs

After that, this PR should be good to merge. 🚀

Thanks for reviewing this!

The funny thing is, as the maintainer of the freshly-unretired rust-zopfli Fedora package, I have no need for the things removed in this PR either—but I would benefit from having the tests in the crate, since I am required to package from the crate rather than from a GitHub archive. I understand the reasons for not wanting to do that, though.

@kornelski
Copy link
Collaborator

@musicinmybrain crates.io packages include .cargo_vcs_info.json file that has a commit hash saved at the time of packaging, and present only if the working copy was clean. Together with repository URL in Cargo.toml that's a pretty decent link between published packages and their git counterparts. Could Fedora allow such association for getting the tests?

Cargo can't use tests from crates.io packages, and doesn't even verify that they compile when making packages. This is effectively an unsupported workflow, and files that weren't meant to be there.

@musicinmybrain
Copy link
Contributor Author

@musicinmybrain crates.io packages include .cargo_vcs_info.json file that has a commit hash saved at the time of packaging, and present only if the working copy was clean. Together with repository URL in Cargo.toml that's a pretty decent link between published packages and their git counterparts. Could Fedora allow such association for getting the tests?

I wonder if what you suggest could be a useful option for our rust2rpm tool. I’ll bring it up with the primary maintainer and see if it goes anywhere.

I forgot to say that, as it is, there are usually still some doc-tests that we can run in the main sources, which gives us some confidence in the package.

Either way, this PR should be ready to merge.

@kornelski kornelski merged commit 2acd4c3 into zopfli-rs:main Feb 9, 2024
3 checks passed
@AlexTMjugador
Copy link
Member

I forgot to say that, as it is, there are usually still some doc-tests that we can run in the main sources, which gives us some confidence in the package.

In the case of this package, there is actually a property test that in the lib.rs file that asserts that compressed data can be decompressed back to the same input, so I'd say that indeed can be used to have a minimum confidence that the package is not terribly broken.

I was going to merge this PR now, but @kornelski beat me to it - thank you both 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants