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

Add some missing LICENSE files to crates, and add CI to verify all crates have LICENSE files. #4664

Closed
wants to merge 2 commits into from

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Aug 9, 2022

Fixes #3761 (again).

I'm not sure about adding LICENSE to all of these crates, but it seems that it
doesn't hurt exactly (all the code is covered under the license regardless,
as we have LICENSE at the root of the repo), and it is easier than trying to
distinguish based on publish = directives or whatnot.

@alexcrichton
Copy link
Member

Personally I do not want to organize the repository like this. I don't think it's useful to have this copied all over the place and even with CI checking it that just seems like it'll be more annoying than anything else.

IIRC Cargo does processing of a license-file directive and puts it into the root crate, so could that be investigated instead of copying the files around?

@cfallin
Copy link
Member Author

cfallin commented Aug 9, 2022

I tend to agree, but downstream packagers want this -- @decathorpe in #3761 needs LICENSE files in each crate (or at least the published ones) for packaging in Fedora (IIRC?).

@decathorpe, would it be acceptable instead to remove all LICENSE files except for the root one, and then update your packaging setup to refer to that?

@decathorpe
Copy link

If I understand correctly, "license-file" is only supposed to be used when using a non-standard license, i.e. the two fields in Cargo.toml are mutually exclusive:

https://doc.rust-lang.org/cargo/reference/manifest.html#the-license-and-license-file-fields

Concerning inclusion of the license text in individual crates: Lawyers / people who understand licenses better than me explained it this way: Some software licenses (including Apache-2.0 and MIT, the de-facto standard licenses for Rust projects), include a requirement that redistributed sources MUST contain a copy of the actual license text (i.e. the LICENSE file).

So, since you are redistributing this project as individual crates on crates.io (and subsequently, Linux distributions might create and redistribute packages for those crates), they need to include license file(s) to be in compliance with the license that you yourselves have chosen for your project.

It also looks like you added copies of the LICENSE files in some places where they are actually not needed (for example, workspace directories that don't contain crates, or "internal" crates that are not published individually).

And, assuming that the operating system that "cargo publish" is run on for this project supports symbolic links, you don't actually need to include full copies of the file everywhere, but a symbolic link is sufficient for cargo to include it.

@alexcrichton
Copy link
Member

I understand that license and license-file are typically mutally exclusive. Your packaging use case does not appear to respect license = "..." in the manifest and requires that some file is present. We do not want to remove license = "..." for Rust-based tooling reasons. I'm proposing a compromise where, if you're willing to put in the work, license-file annotations are added which all point to the root LICENSE in the repository which I believe Cargo will then copy to the crate.

@decathorpe
Copy link

Your packaging use case does not appear to respect license = "..." in the manifest and requires that some file is present.

Our tooling for creating RPM packages does read the package.license value from Cargo.toml, and uses that value to populate the License field of packages.

But even if that were not the case, and the license string was determined by asking an oracle, the problem is the missing license text / file:

c.f. https://choosealicense.com/licenses/apache-2.0/ at 4. Redistribution, (a):

You must give any other recipients of the Work or Derivative Works a copy of this License

I don't see a distinction between redistribution via crates.io and redistribution by linux distributions here (other than the fact that in the first case, you're violating your own license terms, while in the second case, it usually prevents linux distributions from redistributing your code), so the problem is definitely not specific to Linux distribution packages.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language wasmtime:c-api Issues pertaining to the C API. wasmtime:docs Issues related to Wasmtime's documentation labels Aug 9, 2022
@github-actions
Copy link

github-actions bot commented Aug 9, 2022

Subscribe to Label Action

cc @cfallin, @fitzgen, @peterhuene

This issue or pull request has been labeled: "cranelift", "isle", "wasmtime:c-api", "wasmtime:docs"

Thus the following users have been cc'd because of the following labels:

  • cfallin: isle
  • fitzgen: isle
  • peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@cfallin
Copy link
Member Author

cfallin commented Aug 10, 2022

@decathorpe understood that we need to include the license file, one way or another, in the tarball that is uploaded to crates.io. I think this is covered by what @alexcrichton was proposing:

I'm proposing a compromise where, if you're willing to put in the work, license-file annotations are added which all point to the root LICENSE in the repository which I believe Cargo will then copy to the crate.

I just checked with cargo package locally and in a crate with license and license-file attributes, with the latter pointing out-of-tree (to the parent directory), the following happens:

  • The package does include the license file; it is copied in by Cargo.
  • However, Cargo emits a warning:
       Packaging a v0.0.1 (/Users/cfallin/t/a)
       Verifying a v0.0.1 (/Users/cfallin/t/a)
    warning: only one of `license` or `license-file` is necessary
    `license` should be used if the package license can be expressed with a standard SPDX expression.
    `license-file` should be used if the package uses a non-standard license.
    See https://doc.rust-lang.org/cargo/reference/manifest.html#the-license-and-license-file-fields for more information.
    
    and as @alexcrichton notes, we cannot drop the license attribute for a bunch of reasons (tooling reads and depends on it).

@alexcrichton, do you know if there is a way to suppress this warning?

@alexcrichton
Copy link
Member

I don't know how to suppress that myself, and that sounds like an issue for upstream Cargo.

@cfallin
Copy link
Member Author

cfallin commented Aug 10, 2022

This looks like rust-lang/cargo#8537, which is closed with a "we won't support this" disposition -- see this comment in particular. Their reasoning is that adding both keys (license and license-file) could be ambiguous, and Cargo doesn't want the complexity, and tools on top of Cargo should be responsible for gathering license files for standard licenses.

(As an aside, I bet there are a whole bunch of crates on crates.io that have a standard SPDX license expression and don't include the file itself; @decathorpe have you looked at this much for other crates?)

The person who opened that Cargo issue cc'd tokio-rs/tracing#842 where the tracing project had exactly the same discussion that we're having here, and concluded that they just needed to duplicate LICENSE into every crate.

So it seems that's kind of our only option to (i) satisfy legal requirements and (ii) work with Cargo. I'll pare this PR down to just adding LICENSE in the two new crates, and I'll either see if I can find a way to hack the CI license-is-present check into the publish.rs script for just published crates, or omit it for now.

@cfallin
Copy link
Member Author

cfallin commented Aug 10, 2022

@decathorpe I've modified this PR to add LICENSE files just to the six (!) crates we have added recently without LICENSE files, and to add a check to the publish.rs script that runs in our CI to verify publishability.

Unfortunately it looks like wasi-nn is missing a LICENSE file as well, and that's an upstream repo that would need a separate PR to fix and then a PR here to pull in a new submodule version. Would you be willing to do that legwork? If that's merged then CI here should pass and hopefully we can merge this.

@alexcrichton thoughts on this new CI check?

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Personally I don't really want to spend any more time thinking about this. I do not want this state of affairs but not enough to push back on this any further.

@decathorpe
Copy link

Looks good to me, thank you.

@cfallin
Copy link
Member Author

cfallin commented Aug 15, 2022

@decathorpe could you respond to the question:

Unfortunately it looks like wasi-nn is missing a LICENSE file as well, and that's an upstream repo that would need a separate PR to fix and then a PR here to pull in a new submodule version. Would you be willing to do that legwork? If that's merged then CI here should pass and hopefully we can merge this.

I unfortunately can't spend much more time on this either but if you are willing to drive this with the above effort, then I can rebase this and merge when that's done. Thanks!

@decathorpe
Copy link

Sorry, I missed that one.

I can try, but I'm not really interested in wasi-nn as it's dependencies don't look like they'd be available to be.

More important is wasi-common, which also has a problem like this, and it's been ignored for a while: #3912

@cfallin
Copy link
Member Author

cfallin commented Oct 12, 2022

I'm cleaning up old PRs and am going to go ahead and close this for now; this has become out-of-date anyway (new crates in the meantime). @decathorpe please feel free to recreate a new version of the PR as you see fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language wasmtime:c-api Issues pertaining to the C API. wasmtime:docs Issues related to Wasmtime's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cranelift-isle crate is missing the LICENSE file
3 participants