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

Restore license files that were lost in 0.1.22 #202

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

musicinmybrain
Copy link
Contributor

When 6926d6d explicitly listed files to include in published crates, the license files were lost from the crates. This PR restores them by adding them to the include lists.

@musicinmybrain
Copy link
Contributor Author

(It’s a shame that I won’t be able to run tests in Fedora’s rust-jiff package anymore, since we must package from published crates, and 6926d6d removed files needed to compile and run the tests.)

@BurntSushi
Copy link
Owner

Thanks for catching this! I've updated the PR to add COPYING as well, which includes the dual licensing language.

(It’s a shame that I won’t be able to run tests in Fedora’s rust-jiff package anymore, since we must package from published crates, and 6926d6d removed files needed to compile and run the tests.)

I made this change as a result of a request to remove test data in another crate: BurntSushi/bstr#200

I did so there and nobody complained. So I figured I'd do it here too. I started by just excluding the new tzdata file I added, which is nearly 0.5MB. But if I exclude that, then tests will fail to compile. Then I thought: well, should I either spend the effort to make the tests run even if tzdata isn't present, or should I just remove all test related code from the artifact uploaded to crates.io? I ended up going with the latter primarily because:

  • You are, I believe, literally the first person to ever tell me that you run tests using just the crate artifact. My guess is that this means what you're doing is uncommon and what you're doing is not visible.
  • I'm aware of other crates also excluding test related code in uploaded artifacts. (So what do you do with those crates?)
  • I wondered whether it would be a shame to not run tests, or if it would be a shame for >99% of all downloads to waste bandwidth on something that wasn't going to be used?

@decathorpe
Copy link

I made this change as a result of a request to remove test data in another crate: BurntSushi/bstr#200

To be fair, I caught this in bstr too, I just didn't have time to file an issue / comment on the PR yet. In general, for distribution packages (Fedora and debian, to my knowledge), we use crate sources as distributed on crates.io, and run tests (if possible). I understand that it's a trade-off between making downloads larger for everyone or making it possible to run tests from published artifacts - and that in most cases the balance tips towards making the downloads smaller.

@BurntSushi BurntSushi merged commit 55fea15 into BurntSushi:master Jan 13, 2025
20 checks passed
@BurntSushi
Copy link
Owner

@decathorpe I would like to hear more about your use case. For example, how often do y'all catch regressions as a result of running tests using the crate artifact?

@musicinmybrain
Copy link
Contributor Author

Thanks for catching this! I've updated the PR to add COPYING as well, which includes the dual licensing language.

Good catch – thanks!

* You are, I believe, literally the first person to ever tell me that you run tests using just the crate artifact. My guess is that this means what you're doing is uncommon _and_ what you're doing is not visible.

I would say that what Fedora does – packaging from released crates by policy, while also trying to run tests where practical – is uncommon.

* I'm aware of other crates also excluding test related code in uploaded artifacts. (So what do you do with those crates?)

We understand that it’s relatively common to omit separate test files and/or test data, and so we run tests on a best-effort basis. Where it’s too hard to make it happen, we’re just checking that the crate builds. We understand that not always being able to run tests is a natural result of the interaction between our policies and common practices in the Rust community.

In some cases, we can make the tests work by adding an additional source archive, like the archive from GitHub. This is allowed by our policies as long as the GitHub archive is provably used only for testing, and doesn’t contribute to the crate library sources we ship in the resulting -devel package. This tends to be a little tedious, so we are more likely to just disable tests when the crate doesn’t contain necessary files for testing, but the extra effort worth it in some cases. This might be one of those cases, and I’ll probably attempt this approach with the next release.

* I wondered whether it would be a shame to not run tests, or if it would be a shame for >99% of all downloads to waste bandwidth on something that wasn't going to be used?

Well – both. The weight of prevailing practice in the Rust community does tend to be on the side of omitting test sources and data in most cases.

@decathorpe I would like to hear more about your use case. For example, how often do y'all catch regressions as a result of running tests using the crate artifact?

I am not answering for @decathorpe here, but in my experience, we catch regressions reasonably frequently: not as often on x86_64, and much more so on our primary architectures that aren’t in most upstream projects’ CI matrices. We have s390x, which is big-endian, we have i686 (only for x86_64 multilib packages, but still built by default), which is 32-bit, and we have ppc64le and aarch64, which don’t produce as many regressions but do have small differences in floating-point implementations and memory models that occasionally reveal bad assumptions.

@BurntSushi
Copy link
Owner

Aye, thanks for the added context! For Jiff at least, CI I believe has all of those targets running via cross.

But all righty, I'll stick to the current trajectory for now. My guess is that I'll probably end up doing this for my other crates, slowly, over time.

@musicinmybrain
Copy link
Contributor Author

In some cases, we can make the tests work by adding an additional source archive, like the archive from GitHub. This is allowed by our policies as long as the GitHub archive is provably used only for testing, and doesn’t contribute to the crate library sources we ship in the resulting -devel package. This tends to be a little tedious, so we are more likely to just disable tests when the crate doesn’t contain necessary files for testing, but the extra effort worth it in some cases. This might be one of those cases, and I’ll probably attempt this approach with the next release.

This approach was successful. I’m preparing to ship rust-jiff-0.1.23 in Fedora, with the tests re-enabled, and everything looks fine.

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.

3 participants