-
Notifications
You must be signed in to change notification settings - Fork 13k
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
generate-copyright: Now generates a library file too. #133208
Conversation
This comment has been minimized.
This comment has been minimized.
It's a bit surprising to me that we commit the JSON file. My expectation was that the only reason we need to commit anything at all is to have a "pre-built" copyright file (in HTML format?) in the repo, which would be easily checkable by people without the need to run bootstrap. So in that case, I'd expect that on CI, we render the HTML file, and check if it is updated in git. Is the JSON actually needed to be in git? |
As I understood it, building the COPYRIGHT-*.html files is trivial using bootstrap, because it's written in Rust. So we can let bootstrap build those. But the license-metadata.json file requires Edit: Also Copyright-*.html contains the results of scraping the cargo dependency tree for the workspace(s), so it would very quickly get out of sync with Cargo.lock, and that could be annoying. license-metadata.json only contains information that is already in git. |
Ah ha ha ha:
|
Bootstrap also requires Python (as of right now), plus a bunch of other dependencies, so that doesn't seem like a gigantic win to me, tbh.
It wouldn't ever get out of sync for the same reason the JSON won't get out of sync - we'd check in CI that it is in fact in sync :) Our Maybe to take a step back - what does this PR try to achieve? My understanding was that we want to get rid of
So from this point of view, it makes more sense to me to commit the human-readable (and partially machine readable) The question is: do we want to generate and commit copyright metadata that is readable by humans, or metadata that is readable by machines, or both? CC @pietroalbini |
From the Zulip:
The main goal of this PR is to generate two COPYRIGHT files, one for the standard library and one for the toolchain as a whole. Tomorrow I will back out the changes in this PR to where the JSON lives so we can have that discussion separately. |
Sorry, I completely missed the sentence about committing the JSON file 🤦♂️ I'm still interested to discuss whether we really need to commit the JSON file, vs just committing something human readable.. |
So my view is you shouldn't commit anything you can reasonably regenerate in CI. So to me the choice is, should we commit nothing, or should we commit the JSON to avoid people needing to have reuse installed. |
I never needed to handle JSON copyright data, but I find it very useful to have a human readable COPYRIGHT file in the root of a repo - it is a standard thing that many repositories have and people kind of expect it will be there, IMO. So it makes sense to me to automatically pregenerate this file. |
The plan is to deploy it with every release, so it's in /usr/local/share/rust/whatever, or wherever your toolchain is installed to. Because it's the product of a relatively expensive complication process, like It's also only really useful to people who don't have the source code checked out. If you have the source code, you have much more fine-grained copyright information - in the source files themselves. This is just a summary of the source tree. |
Sure, but it's still common to have a COPYRIGHT file in the root of a repo anyway. People don't want to go through the source tree to figure out copyright, not to mention that most of our Rust files don't actually have any. And you have created the awesome HTML copyright page, so let's use it and commit it so that it is easily accessible! Anyway, I didn't want to derail this PR, sorry. IMO, we should definitely remove the previous COPYRIGHT file(s) and replace them with automatically generated COPYRIGHT files, commit these, and ensure that they stay fresh in CI. As long as we do that, I don't mind if we also commit the JSON file, even though I find it a bit redundant. |
We only run reuse once, so the output has to be filtered to find only the files that are relevant to the library tree. Outputs build/COPYRIGHT.html and build/COPYRIGHT-library.html.
c9292c3
to
9dfc682
Compare
I dropped the changes regarding committing the JSON file. I can do another PR where I add |
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.
Thank you for reducing the scope of this PR. The code looks good. When I tried to generate the libstd copyright, I got these weird empty entries:
But I suppose it's still the same case as this.
/// Describes a tree of metadata for our filesystem tree | ||
#[derive(serde::Deserialize)] | ||
/// | ||
/// Must match the JSON emitted by the `CollectLicenseMetadata` bootstrap tool. |
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 only noticed now that the definition of these (non-trivial) JSON structures is duplicated here and in collect-license-metadata
. Eventually it would be nice to share these e.g. through the build_helper
crate, but let's not complicate this PR with that.
I'll poke at generate-license-metadata and see if we can make that better. But in general, |
I haven't found an easy way to tell reuse to ignore untracked files, sadly. Anyway, that's not relevant to this PR, that's a separate issue. |
If only CI runs the program, and CI doesn't have untracked files, it'll be moot. |
Indeed. Thank you! @bors r+ |
…Kobzol generate-copyright: Now generates a library file too. We only run reuse once, so the output has to be filtered to find only the files that are relevant to the library tree. Outputs COPYRIGHT.html and COPYRIGHT-library.html. The license-metadata.json file is also now in the tree. We need a CI tool to check that it's correct. r? kobzol Remaining steps: * [ ] Teach CI to double-check the license-metadata.json file is correct * [ ] Add the COPYRIGHT.html and COPYRIGHT-license.html to the releases
Back when this effort started @Mark-Simulacrum stated that he didn't want to require anyone running Also, the frequency of changes to the JSON and the HTML is vastly different. The JSON will change very rarely (only when we add files with different copyright to this repo), while the HTML will be regenerated fairly often (every time a dependency is bumped in a lockfile. Committing the HTML increases the annoyance for contributors (who will have to run an extra step) and requires everyone who updates dependencies to have REUSE installed. |
Hmm, but the difference in that is not JSON vs HTML, but rather the fact that the JSON only contains licenses of in-tree source code, while the HTML right now contains also out-of-tree licenses, so strictly more data. We could also generate the HTML file with only in-tree data, which would make it much smaller. In other words, the JSON file is not the minimal set of information needed to generate EDIT: Ah, I think I understand now. It is the "minimal set of information needed to generate COPYRIGHT, if you don't want to install REUSE". Well, I understand that, but it seems to me that this is quite a bit of complexity required just that you can avoid typing FWIW, REUSE is IMO quite easy to install. You already need to have Python installed to even run bootstrap, and some Python packages are needed to run extra tidy checks, which can also trip you up on CI after making certain changes. |
Sure, if we are ok with more people installing REUSE it can be less complexity, the choice was made since there was a strong preference not to have people install it. I guess the three options are:
In the end it's all tradeoffs 🙂 |
…mpiler-errors Rollup of 8 pull requests Successful merges: - rust-lang#132090 (Stop being so bail-y in candidate assembly) - rust-lang#132658 (Detect const in pattern with typo) - rust-lang#132911 (Pretty print async fn sugar in opaques and trait bounds) - rust-lang#133102 (aarch64 softfloat target: always pass floats in int registers) - rust-lang#133159 (Don't allow `-Zunstable-options` to take a value ) - rust-lang#133208 (generate-copyright: Now generates a library file too.) - rust-lang#133215 (Fix missing submodule in `./x vendor`) - rust-lang#133264 (implement OsString::truncate) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#133208 - ferrocene:split-copyright-html, r=Kobzol generate-copyright: Now generates a library file too. We only run reuse once, so the output has to be filtered to find only the files that are relevant to the library tree. Outputs COPYRIGHT.html and COPYRIGHT-library.html. The license-metadata.json file is also now in the tree. We need a CI tool to check that it's correct. r? kobzol Remaining steps: * [ ] Teach CI to double-check the license-metadata.json file is correct * [ ] Add the COPYRIGHT.html and COPYRIGHT-license.html to the releases
We only run reuse once, so the output has to be filtered to find only the files that are relevant to the library tree.
Outputs COPYRIGHT.html and COPYRIGHT-library.html.
The license-metadata.json file is also now in the tree. We need a CI tool to check that it's correct.
r? kobzol
Remaining steps: