wasmtime icon indicating copy to clipboard operation
wasmtime copied to clipboard

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

Open cfallin opened this issue 3 years ago • 13 comments
trafficstars

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.

cfallin avatar Aug 09 '22 19:08 cfallin

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?

alexcrichton avatar Aug 09 '22 19:08 alexcrichton

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?

cfallin avatar Aug 09 '22 19:08 cfallin

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.

decathorpe avatar Aug 09 '22 19:08 decathorpe

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.

alexcrichton avatar Aug 09 '22 20:08 alexcrichton

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.

decathorpe avatar Aug 09 '22 20:08 decathorpe

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.

github-actions[bot] avatar Aug 09 '22 23:08 github-actions[bot]

@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?

cfallin avatar Aug 10 '22 03:08 cfallin

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

alexcrichton avatar Aug 10 '22 15:08 alexcrichton

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 avatar Aug 10 '22 16:08 cfallin

@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?

cfallin avatar Aug 10 '22 16:08 cfallin

Looks good to me, thank you.

decathorpe avatar Aug 15 '22 13:08 decathorpe

@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!

cfallin avatar Aug 15 '22 16:08 cfallin

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: https://github.com/bytecodealliance/wasmtime/issues/3912

decathorpe avatar Aug 15 '22 18:08 decathorpe

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.

cfallin avatar Oct 12 '22 02:10 cfallin