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

cargo vendor does not include license-file if not in package list #9575

Open
zbraniecki opened this issue Jun 11, 2021 · 14 comments
Open

cargo vendor does not include license-file if not in package list #9575

zbraniecki opened this issue Jun 11, 2021 · 14 comments
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-vendor E-medium Experience: Medium S-triage Status: This issue is waiting on initial triage.

Comments

@zbraniecki
Copy link

Describe the problem you are trying to solve

Mono-repos usually have a single LICENSE file in the top directory and they reference it from all crates via some license-file = "../../LICENSE".

At the moment, cargo will not package file from such path.

Since the license file needs to be packaged into the crate for it to be able to be vendored in into other projects.

We encountered that problem when trying to vendor in ICU4X (rust mono repo) into Firefox.

In order to comply with the vendor policy of Mozilla, we need to duplicate the LICENSE file to each component of the mono repo and introduce a maintenance script to ensure the licenses stay in sync.

See the issue, and the PR.

Describe the solution you'd like

It would help such projects if files referenced in Cargo.toml were packaged even if they live in the parent directory to the crate root.

The file could be copied to the root directory of the package.

Notes

If that's impossible, I'd at least suggest displaying a warning that cargo skips packaging of the file.

@zbraniecki zbraniecki added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Jun 11, 2021
@ehuss
Copy link
Contributor

ehuss commented Jun 11, 2021

Can you maybe clarify with an example? The license-file should be packaged:

[package]
name = "foo"
version = "0.1.0"
edition = "2018"
license-file = "../../LICENSE-MIT"
> cargo package
warning: manifest has no description, documentation, homepage or repository.
See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
   Packaging foo v0.1.0 (/Users/eric/Proj/rust/cargo/scratch/foo)
   Verifying foo v0.1.0 (/Users/eric/Proj/rust/cargo/scratch/foo)
   Compiling foo v0.1.0 (/Users/eric/Proj/rust/cargo/scratch/foo/target/package/foo-0.1.0)
    Finished dev [unoptimized + debuginfo] target(s) in 0.76s
> tar -tzf target/package/foo-0.1.0.crate
foo-0.1.0/Cargo.lock
foo-0.1.0/Cargo.toml
foo-0.1.0/Cargo.toml.orig
foo-0.1.0/LICENSE-MIT
foo-0.1.0/src/main.rs

@zbraniecki
Copy link
Author

@dminor should be able to!

@dminor
Copy link

dminor commented Jun 11, 2021

Yes, sorry, you're quite right, packaging is working just fine, I'm just not seeing the license file after running cargo vendor.

I'm testing with the icu_locid crate, 0.2.0. I've pulled down the package directly and verified the LICENSE file is present in what is on crates.io.

Do we need to do something different with the package configuration for cargo vendor to work in this situation?

@ehuss
Copy link
Contributor

ehuss commented Jun 14, 2021

Nah, it's just an issue with cargo vendor.

It may be tricky to fix, since cargo package needs to handle rewriting the path to be relative to the package, and the package listing code just returns a simple list of paths, with the assumption they are all inside the package root. If anyone wants to dig into it, the cargo package code is here, and the vendoring code is here. However, cargo package uses the same list_files() function, so just adding the license-file there will probably cause problems (and, list_files assumes all returned paths are relative to the package root).

@ehuss ehuss added Command-vendor E-medium Experience: Medium labels Jun 14, 2021
@ehuss ehuss changed the title Enable packaging of files from parent directory cargo vendor does not include license-file if not in package list Jun 14, 2021
@zbraniecki
Copy link
Author

Could it be solved with includes pointing at the file LICENSE.txt that in the package is in the root? From the vendor perspective there's no difference if the file was originally in the root dir before packaging, or some super-root, right?

@ehuss
Copy link
Contributor

ehuss commented Jun 14, 2021

If you mean, could icu_locid add either ../../LICENSE.txt or /LICENSE.txt to include, I think neither will work (since it doesn't allow outside paths, and the file doesn't exist in the root of the package). I can't think of any easy workarounds, unfortunately.

@zbraniecki
Copy link
Author

Ah, so maybe I don't understand how cargo vendor work. I thought it operates based on packaged zip file in which LICENSE.txt is in the root, but now that I reason about it, it also can vendor in from git URL in which case i'd have to find ../../LICENSE in the Cargo.toml and navigate its way to it, right?

@ehuss
Copy link
Contributor

ehuss commented Jun 14, 2021

Yea. It starts with the packaged zip file, but then uses the include list to drive what to copy. This was mainly added to support git dependencies. There's more history about why it does it at alexcrichton/cargo-vendor#137, alexcrichton/cargo-vendor#139. Another option would be to maybe have cargo vendor only use list_files for git dependencies? One could say you still want the LICENSE file for git dependencies, too, though.

@Manishearth
Copy link
Member

Yea. It starts with the packaged zip file, but then uses the include list to drive what to copy

Perhaps we can explicitly include the LICENSE key? It seems pretty important to not drop that.

@ehuss
Copy link
Contributor

ehuss commented Jun 16, 2021

Yea, that would probably be the best fix. As mentioned above, there are some links there if someone is interested into digging into it. I'm a little uncomfortable duplicating that logic in multiple places, but it can't be put directly into list_files due to relative paths. It is not obvious to me how to structure that in a clean fashion.

@Manishearth
Copy link
Member

@ehuss isn't it a local path once it's in the package?

@ehuss
Copy link
Contributor

ehuss commented Jun 16, 2021

Sort of. Packages published prior to 1.43 won't have rewritten paths or included license-files, so it would need to handle that.

list_files is used for several purposes (packaging, vendoring, build script and rustdoc change detection). list_files works before files are placed into a package (before relative rewriting), and assumes all paths are under the package root. It would be a problem for it to start returning paths outside of the package.

So, some options I can think of:

  1. Special-case cargo vendor. When it sees a license-file in Cargo.toml, it could make sure it gets included (if it exists). This has several downsides, such as complexity, dealing with older versions, getting out of sync in the future when Cargo adds more special files, and an inability to rewrite Cargo.toml in git dependencies.
  2. Have list_paths return a more sophisticated response, such as a list of enums indicating different file kinds. This has all the drawbacks of the previous option, and adds a lot more complexity.
  3. Switch cargo vendor to copy the source as-is for registry dependencies (and use list_paths for git and path dependencies, which is sub-optimal, but 🤷 ).

None of these options sound good, but the last seems possibly better now that I've written them out.

@Manishearth
Copy link
Member

Personally I was assuming that #3 is already what happens before learning about the implementation from this thread 😄

@kvark
Copy link

kvark commented Mar 30, 2022

Based on Fuchsia requirements (see gfx-rs/wgpu#2561), It would help if cargo vendor was including a license file in the package even if license = "..." is used instead of license-file = "...". This is probably more involved though, but it's not clear to me if this logic could or should live elsewhere outside of cargo vendor.

@epage epage added the S-triage Status: This issue is waiting on initial triage. label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-vendor E-medium Experience: Medium S-triage Status: This issue is waiting on initial triage.
Projects
None yet
Development

No branches or pull requests

6 participants