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

warning: File system loop found: xxx printed when running cargo run on file not ever needed by cargo #12600

Open
lucacasonato opened this issue Aug 30, 2023 · 15 comments
Labels
A-build-scripts Area: build.rs scripts C-bug Category: bug S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request.

Comments

@lucacasonato
Copy link

Problem

cargo run prints a warning: File system loop found: xxx warning message on every build, for a symlink that is not ever needed by Cargo.

This file is in a fixture/ directory. The fixture exists specifically to test the code with this file system loop case.

Steps

  1. Clone https://github.com/denoland/deno.git
  2. Install protoc and cmake
  3. cargo run

Observe:

warning: File system loop found: /path/to/deno/test_util/std/wasi/testdata/fixtures/symlink_to_parent_directory/fixtures/symlink_to_parent_directory points to an ancestor /path/to/deno/test_util/std/wasi/testdata/fixtures/symlink_to_parent_directory

Possible Solution(s)

No response

Notes

This did not occur in 1.71.0.

Version

cargo 1.72.0 (103a7ff2e 2023-08-15)
release: 1.72.0
commit-hash: 103a7ff2ee7678d34f34d778614c5eb2525ae9de
commit-date: 2023-08-15
host: aarch64-apple-darwin
libgit2: 1.6.4 (sys:0.17.2 vendored)
libcurl: 7.88.1 (sys:0.4.63+curl-8.1.2 system ssl:(SecureTransport) LibreSSL/3.3.6)
ssl: OpenSSL 1.1.1u  30 May 2023
os: Mac OS 13.4.1 [64-bit]
@lucacasonato lucacasonato added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Aug 30, 2023
@weihanglo
Copy link
Member

Hmm… I don't know how 1.71.0 and 1.72.0 differ.
Could you try adding exclude = ["testdata/"] to test_util/Cargo.toml? I feel like this may solve the problem.

@epage
Copy link
Contributor

epage commented Aug 30, 2023

For background, the message is coming from the ignore crate which we use to walk path sources to fingerprint them for detecting rebuilds.

@weihanglo weihanglo added S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. and removed S-triage Status: This issue is waiting on initial triage. labels Aug 30, 2023
@ijackson
Copy link
Contributor

ijackson commented Feb 2, 2025

IMO symlinks pointing to ancestor directories is normal on Unix. This ought not to generate a warning.

Is there a way to completely suppress it?

@ijackson
Copy link
Contributor

ijackson commented Feb 2, 2025

Also, cargo should be following symlinks which are not relevant to its own operation. In my project I'm getting warnings about "loops" in my maint/ directory, which cargo ought not to be looking at.

A working tree might contain symlinks to hazardous or untrusted data (for example, in test temporary directories, or test cases). cargo ought not to be scanning the whole directory tree.

Tools that scan "every file" in a subtree ought not to be following symlinks at all.

@epage epage added the A-build-scripts Area: build.rs scripts label Feb 3, 2025
@epage
Copy link
Contributor

epage commented Feb 3, 2025

Also, cargo should be following symlinks which are not relevant to its own operation. In my project I'm getting warnings about "loops" in my maint/ directory, which cargo ought not to be looking at.

I believe this is only happening when using build scripts which makes things a bit fuzzier. #9931 does not help.

@weihanglo
Copy link
Member

weihanglo commented Feb 4, 2025

The warning was introduced via #10214. If it is really that common we should be open to adjust #10214 (comment). However, that means we either need to reopen #9528, or has a debug! log helping future debugging. I lean to the logging approach a bit more, though people might still be confused because there will be no warning in the first place. See

@epage
Copy link
Contributor

epage commented Feb 4, 2025

@ijackson

IMO symlinks pointing to ancestor directories is normal on Unix. This ought not to generate a warning.

Could you provide context to this. In what way are they common? What is the use case, particularly as it relates to cargo source trees?

@ijackson
Copy link
Contributor

ijackson commented Feb 4, 2025

Could you provide context to this. In what way are they common?

I find this question perplexing. I can try to expand:

I have been working with Unix systems since 1989. In my experience symlinks to ancestor directories are common.

The usual reason is simply (i) we use symlinks a lot, whenever we have some situation where a file or directory is being searched or used or most conveniently found in one place, but we want it to be physically located elsewhere. Reasons might include: so that it is updated as part of the right subsystem; to put it on the right filesystem volume with the right storage properties; to put it on the right fileserver; to refer to objects from a superior security context; to refer to a separately managed (sub)project; compatibility reasons. Plus, (ii) often the actual location wants to be an ancestor of the symlink, and this is normal and supported on Unix and doesn't cause any problems. Conversely tree walking tools on Unix don't generally descend through symlinks. (Some older tools need to be given a special option.)

One example is that symlinks to . are a common approach to flattening otherwise-unwieldy directory structures. (For human convenience, for scripting convenience, for homogeneity with competing layout notions, or for transitional compatibility.)

What is the use case, particularly as it relates to cargo source trees?

The one that happened to me yesterday is this:

https://gitlab.torproject.org/tpo/core/rust-maint-common/-/tree/main

You'll observe that it has a symlink maint -> .. Here it is in use:

https://salsa.debian.org/dgit-team/tag2upload-service-manager/-/tree/main/maint?ref_type=heads

For reasons I don't understand, cargo check on that project doesn't produce the warnngs. But:

cargo +nightly doc --locked
warning: File system loop found: /volatile/rustcargo/Rustup/Tag2upload/tag2upload-service-manager/maint/common/common points to an ancestor /volatile/rustcargo/Rustup/Tag2upload/tag2upload-service-manager/maint/common
warning: File system loop found: /volatile/rustcargo/Rustup/Tag2upload/tag2upload-service-manager/maint/common/maint points to an ancestor /volatile/rustcargo/Rustup/Tag2upload/tag2upload-service-manager/maint/common
warning: File system loop found: /volatile/rustcargo/Rustup/Tag2upload/tag2upload-service-manager/maint/rust-maint-common/common/common points to an ancestor /volatile/rustcargo/Rustup/Tag2upload/tag2upload-service-manager/maint/rust-maint-common/common
warning: File system loop found: /volatile/rustcargo/Rustup/Tag2upload/tag2upload-service-manager/maint/rust-maint-common/common/maint points to an ancestor /volatile/rustcargo/Rustup/Tag2upload/tag2upload-service-manager/maint/rust-maint-common/common
warning: File system loop found: /volatile/rustcargo/Rustup/Tag2upload/tag2upload-service-manager/maint/rust-maint-common/maint/common points to an ancestor /volatile/rustcargo/Rustup/Tag2upload/tag2upload-service-manager/maint/rust-maint-common/maint
warning: File system loop found: /volatile/rustcargo/Rustup/Tag2upload/tag2upload-service-manager/maint/rust-maint-common/maint/maint points to an ancestor /volatile/rustcargo/Rustup/Tag2upload/tag2upload-service-manager/maint/rust-maint-common/maint
 Documenting unicode-ident v1.0.16

IMO cargo doc has no business grobbling around in maint/ at all, let alone complaining about these perfectly fine symlinks.

@weihanglo
Copy link
Member

IMO cargo doc has no business grobbling around in maint/ at all, let alone complaining about these perfectly fine symlinks.

rustdoc doesn't generate any depinfo file for Cargo to track dependencies for rebuild detection. The current workaround is doing a full scan of your package to get mtime of each file. See the PR description in #9528.

@ijackson
Copy link
Contributor

ijackson commented Feb 4, 2025

Oh, and:

@ijackson

What is the use case, particularly as it relates to cargo source trees?

Note that it is not reasonable to assume that a project source tree being processed by cargo is (just) "a cargo source tree".

Projects typically contain information and source code aimed at multiple different kinds of tools, of which cargo is only one. Even pure Rust projects typicaly contain CI metadata, project management scripts, test case including test data, etc. More sophisticated projects will contain websites, documentation, and subprograms written in other languages.

In summary, cargo should do its job, but mind its own business. That's part of playing well with others. It's also part of being a robust and safe tool.

Tree contents that don't have names indicating that they are for cargo, may be worse than confusing to cargo. They may be hazardous - possibly much more hazardous than the mildly anomalous test data in the OP's test cases.

@ijackson
Copy link
Contributor

ijackson commented Feb 4, 2025

rustdoc doesn't generate any depinfo file for Cargo to track dependencies for rebuild detection. The current workaround is doing a full scan of your package to get mtime of each file. See the PR description in #9528.

How exciting.

I see that #9528 is closed. It's a shame that the proper solution remains unimplemented since 2021. Is there a ticket to track this defect?

@weihanglo
Copy link
Member

Is there a ticket to track this defect?

They are

@epage
Copy link
Contributor

epage commented Feb 4, 2025

The usual reason is simply (i) we use symlinks a lot, whenever we have some situation where a file or directory is being searched or used or most conveniently found in one place, but we want it to be physically located elsewhere. Reasons might include: so that it is updated as part of the right subsystem; to put it on the right filesystem volume with the right storage properties; to put it on the right fileserver; to refer to objects from a superior security context; to refer to a separately managed (sub)project; compatibility reasons. Plus, (ii) often the actual location wants to be an ancestor of the symlink, and this is normal and supported on Unix and doesn't cause any problems.

@ijackson my question was not "why use symlinks" but "why use recursive symlinks in Rust projects". Most of the answer you gave seems to be about symlinks more generally. The examples come closest.

One example is that symlinks to . are a common approach to flattening otherwise-unwieldy directory structures. (For human convenience, for scripting convenience, for homogeneity with competing layout notions, or for transitional compatibility.)

I don't quite get the flattening aspect. I understand compatibility though that would only be in transition and so not the most important for resolving this. The other case seems to be about providing a default for a tool that can be redirected?

Conversely tree walking tools on Unix don't generally descend through symlinks. (Some older tools need to be given a special option.)

...

In summary, cargo should do its job, but mind its own business. That's part of playing well with others. It's also part of being a robust and safe tool.

Following symlinks is Cargo doing its job. We're fingerprinting all of the input for a build. If we stopped at symlinks, we wouldn't know if the target content changed.

What could be done better is for those linked issues to be fixed so we only scan explicit or implicit inputs to a build.

from #15141 (comment)

Currently I'm getting these warnings but I just want them suppressed, not to become fatal. See #12600

We'd be blocked on #12235

@ijackson
Copy link
Contributor

ijackson commented Feb 4, 2025

What could be done better is for those linked issues to be fixed so we only scan explicit or implicit inputs to a build.

I agree that this is the right resolution. It would resolve the situation completely. Thanks.

Currently I'm getting these warnings but I just want them suppressed, not to become fatal. See #12600

We'd be blocked on #12235

Right. I can live with the warnings.

In the meantime, whether to follow symlinks while scanning the whole tree is still a question, I think. In the usual case, symlinks will point to other parts of the same build tree. Having them point "out of the tree" is going to be less likely.

If we're scanning the whole tree, in-tree symlink targets don't need to be scanned at all. After all, we'll find the files at the symlink's target when we scan them directly, under their real paths.

The problem is with out-of-tree symlinks. I think scanning these is hazardous. They might point anywhere and having cargo follow those links is quite unexpected. On the other hand, some folks may deliberately make links pointing out of tree as a kind of ersatz git subtree.

I suggest the following resolution for now:

  • When scanning, don't follow symlinks that point to within the area we're scanning anyway. (Look at the timestamp on the symlink in case the link itself has changed.) Don't print any warning for this situation.

  • If there are any out of tree symlinks, offer these options: 1. rebuild everything (treat the symlink target as changed); 2. don't follow the link and don't scan it; 3. do follow the link and scan whatever directory hierarchy it points to. I think option 1 is the only safe default. It should generate a warning.

@ijackson
Copy link
Contributor

This troublesome warning also happens if you have a build.rs that doesn't emit rerun-if-changed lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-scripts Area: build.rs scripts C-bug Category: bug S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants