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 publish excludes files called target #12790

Closed
KarelPeeters opened this issue Oct 7, 2023 · 5 comments · Fixed by #12944
Closed

Cargo publish excludes files called target #12790

KarelPeeters opened this issue Oct 7, 2023 · 5 comments · Fixed by #12944
Assignees
Labels
C-bug Category: bug Command-package Command-publish E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@KarelPeeters
Copy link
Contributor

KarelPeeters commented Oct 7, 2023

Problem

When publishing or packaging a crate that contains a file called target (different from the typical root target folder), the file is not included. This happens even though the target file is not ignored by either .gitignore or .git/info/exclude. It's also not a glob evaluation issue, this is still an issue if .gitignore is completely empty.

This also only happens if the target file is currently tracked in git. This is not that important by itself, but it might help figuring out what exactly causes the bug.

Steps

I've made a repository to reproduce this bug: https://github.com/KarelPeeters/cargo_bug_package_target

The steps to follow:

> cargo package --list --allow-dirty
.gitignore
Cargo.lock
Cargo.toml
Cargo.toml.orig
README.md
Readme.md
data/not_target
src/main.rs

note that data/target is missing, this is the main bug.

  • ensure that target is not "tracked" any more by moving or deleting .git entirely
> mv .git .not_git
  • check the list of files that will be packaged again
> cargo package --list --allow-dirty
Cargo.lock
Cargo.toml
Cargo.toml.orig
README.md
Readme.md
data\not_target
data\target
src\main.rs

Note that data/target is not missing anymore, this shows that the bug only happens for tracked files.

Possible Solution(s)

I assume this is caused by some bad interaction in build_ar_list between the VCSInfo stuff and the hardcoded check for a filename of "target" here: https://github.com/rust-lang/cargo/blob/master/src/cargo/ops/cargo_package.rs#L934

I'm not sure what much of the context there is trying to do, so I don't know what the right fix is.

One issue to keep in mind is that multiple crates that share a repository while not being a workspace can have multiple target folders. So a simple check like "only ignore the root target directory" is also not good enough. There should be existing infrastructure to figure out what the real target directory is though, since cargo clean (like many other subcommands) needs that.

Notes

I noticed this when trying to vendor some cuda headers and associated files, which is neccesary to get docs.rs to build one of my crates correctly.

One of these files happens to be called target.

Here it is in my github repository: https://github.com/KarelPeeters/Kyanite/tree/1eb40f2dde70d3432f218bfbaa5c7d37eebbd394/kn-cuda-sys/doc_headers/cuda_include/nv
But the same file is missing on docs.rs and crates.io: https://docs.rs/crate/kn-cuda-sys/0.4.1/source/doc_headers/cuda_include/nv/

Version

cargo 1.71.0 (cfd3bbd8f 2023-06-08)
release: 1.71.0
commit-hash: cfd3bbd8fe4fd92074dfad04b7eb9a923646839f
commit-date: 2023-06-08
host: x86_64-pc-windows-msvc
libgit2: 1.6.4 (sys:0.17.1 vendored)
libcurl: 8.0.1-DEV (sys:0.4.61+curl-8.0.1 vendored ssl:Schannel)
os: Windows 10.0.22000 (Windows 11 Core) [64-bit]
@KarelPeeters KarelPeeters added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Oct 7, 2023
@KarelPeeters
Copy link
Contributor Author

Sadly naming the file to something else and then copying it over in build.rs doesn't work either, since docs.rs only exposes a readonly filesystem: https://docs.rs/crate/kn-cuda-sys/0.4.2/builds/934175

KarelPeeters added a commit to KarelPeeters/Kyanite that referenced this issue Oct 8, 2023
@weihanglo
Copy link
Member

Thanks for the report. Haven't got time running the repro but I am guessing this piece of code is the culprit:

// The `target` directory is never included.
Some("target") => continue,

It may need to verify if the target directory is exactly a directory. I am happy to mentor and review if someone wants to take a look.

In addition, there is no an official way to detect if a directory is target-dir. A relevant discussion can be found in #12441.

@weihanglo weihanglo added Command-publish Command-package S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review E-easy Experience: Easy and removed S-triage Status: This issue is waiting on initial triage. labels Oct 8, 2023
@KarelPeeters
Copy link
Contributor Author

KarelPeeters commented Oct 8, 2023

It may need to verify if the target directory is exactly a directory

Checking if it's a directory still leaves part of the issue, since it potentially still makes sense to have a resource directory called target.

In addition, there is no an official way to detect if a directory is target-dir. A relevant discussion can be found in #12441.

Thinking about it some more, why is knowing the true target directory so hard? Is there anything wrong with this logic?

  • if the current crate is part of a workspace it's WORKSPACE_ROOT/target
  • otherwise it's CURRENT_CRATE/target

I am guessing this piece of code is the culprit:

// The `target` directory is never included.
Some("target") => continue,

That line is another good candidate, I wonder why cargo package has its own similar file-listing code.

I am happy to mentor and review if someone wants to take a look.

I'm willing to give this issue a shot. I'll get started building cargo and seeing if I can figure out the exact cause, and then start writing a test for this.

@epage
Copy link
Contributor

epage commented Oct 9, 2023

Finding the target directory can be challenging because its configurable, from call to call even.

Another challenging part is that packages might not be in a workspace (or they might have not been in a workspace but now are).

Most likely, the safest option is to only filter out target if its in the package root. The subpackage code will already take care of other target/s

Note that we have two code paths for enumerating source for a package. The one @weihanglo linked and

// Skip root Cargo artifacts.
. This later one already does a depth check. I wonder what other differences there are.

I assume this is caused by some bad interaction in build_ar_list between the VCSInfo stuff and the hardcoded check for a filename of "target" here: https://github.com/rust-lang/cargo/blob/master/src/cargo/ops/cargo_package.rs#L934

Two things about that check you linked

  • It appears to only be used during the verification step, when extracting the .crate and building it. It doesn't need the same file list as packaging but is trying to do a directory diff.
  • It already only excludes target if its in the root

@linyihai
Copy link
Contributor

linyihai commented Nov 9, 2023

Most likely, the safest option is to only filter out target if its in the package root.

This seems to be the best solution so far. I will make a PR for it. @rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug Command-package Command-publish E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
4 participants