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 non-library sources when uploading to crates.io #4

Closed
wants to merge 1 commit into from
Closed

exclude non-library sources when uploading to crates.io #4

wants to merge 1 commit into from

Conversation

ComputerDruid
Copy link

My main motivation is to remove the test data from /tests/, but I figured I'd clean up the other unneeded things while I was at it.

I care about the testdata because I'm trying to pull this crate into https://cs.opensource.google/fuchsia/fuchsia/+/main:third_party/rust_crates/vendor via cargo vendor, but it's not 100% clear what the license on the testdata is. Excluding it from being uploaded side-steps that problem entirely (and also makes everyone else's downloads slightly smaller)

My main motivation is to remove the test data from `/tests/`, but I
figured I'd clean up the other unneeded things while I was at it.
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether or not we do this, I'd like to clarify the license of the test files. Would you be able to point out which files are at issue?

@ComputerDruid
Copy link
Author

I'm specifically unsure about the *.fst files in tests/fst/ and tests/trie/trie.rs

@dtolnay
Copy link
Owner

dtolnay commented Jun 11, 2022

Got it. Those are generated by https://github.com/BurntSushi/ucd-generate which is dual MIT/Apache-2.0 licensed, same as this crate. Would you be willing to follow up in that repo about how to make the license of generated outputs clearer, maybe by adding license info to the comment at the top of generated Rust source files and emitting a license file next to binary files? Regardless of what parts are published to crates.io, it's still an issue for how they're provided here on GitHub.

The input database's use, according to https://www.unicode.org/faq/unicode_license.html is governed by https://www.unicode.org/copyright.html and https://www.unicode.org/license.txt. In the event that there is any way we are out of compliance with that license, then this PR doesn't fix it because it would apply just as much to this crate's use of the same database for the generated tables in the src directory.

I see that unicode-xid is already imported in the Fuchsia repo in your link, and doesn't do anything in particular to reference the Unicode license / Terms of Use as far as I can tell. If it would make the licensing of the generated tables clearer I would be happy to do better in this crate.

@ComputerDruid
Copy link
Author

Thanks for that information, I actually had missed that src/tables.rs is also generated from the same source. (actually, it would be nice if that file linked to how it was generated, now that I'm thinking about it)

Also, thanks for pointing out unicode-xid being similar, I'm going to reach out to our licensing folks for what to do next. I'll come back to this once I know more.

@ComputerDruid
Copy link
Author

OK, coming back to this now that I've talked to some licensing folks. I think it makes sense to include the unicode license from https://www.unicode.org/license.txt in this crate's licenses.

Also, for unicode-xid, @anp recently opened unicode-rs/unicode-xid#32 to do the same. He also opened rust-lang/rust#98116 for a similar question about libstd.

@dtolnay
Copy link
Owner

dtolnay commented Jul 16, 2022

I published 1.0.2 to crates.io which includes this explanation of the license: https://crates.io/crates/unicode-ident/1.0.2#license along with https://github.com/dtolnay/unicode-ident/blob/1.0.2/LICENSE-UNICODE.

I have also included a new comment at the top of src/tables.rs indicating how it is generated (#6) in response to the suggestion in #4 (comment).

I believe the original motivation behind this PR has been resolved at this point. Do you prefer still moving forward with this? If yes, some further refactoring is going to be required because cargo package (which is a step of cargo publish) cannot handle the current change.

error: failed to verify package tarball

Caused by:
  failed to parse manifest at `target/package/unicode-ident-1.0.2/Cargo.toml`

Caused by:
  can't find `xid` bench at `benches/xid.rs` or `benches/xid/main.rs`. Please specify bench.path if you want to use a non-default path.

The difference in size of unicode-ident.crate with and without the benchmarks and tests is 35K vs 17K which I don't expect would make a difference to anybody in practice.

@ComputerDruid
Copy link
Author

Yep; I'm happy without this at this point. Thanks!

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.

2 participants