Poor error message when target is a broken symlink

[rben01]

Problem

When running cargo build, Cargo tries to output to target. If target is an ordinary file, or a symlink that points to one, this (naturally) fails with

$ cargo build
error: failed to create directory `/path/to/crate/target/debug`

Caused by:
  Not a directory (os error 20)

However, if target is a broken symlink, Cargo prints a much less helpful error message:

$ ln -s /a/b/c target  # assuming /a/b/c doesn't exist on your machine
$ cargo build
error: Not a directory (os error 20)

The error: failed to create directory part of the error message has been lost.

Steps

1. Create a new crate with cargo init
2. Create a broken target symlink with e.g., ln -s /a/b/c target (assuming you don't actually have /a/b/c on your computer)
3. Run cargo build

Possible Solution(s)

Cargo should catch the broken target symlink and print the same error message it would if target were an ordinary file.

Notes

No response

Version

cargo 1.74.0-nightly (2cc50bc0b 2023-08-22)
release: 1.74.0-nightly
commit-hash: 2cc50bc0b63ad20da193e002ba11d391af0104b7
commit-date: 2023-08-22
host: aarch64-apple-darwin
libgit2: 1.6.4 (sys:0.17.2 vendored)
libcurl: 8.1.2 (sys:0.4.65+curl-8.2.1 system ssl:(SecureTransport) LibreSSL/3.3.6)
ssl: OpenSSL 1.1.1u  30 May 2023
os: Mac OS 14.0.0 [64-bit]

[weihanglo]

Thanks for the report. Sounds pretty reasonable to enhance that.

@rustbot label +S-accepted +A-error -S-triage

[linyihai]

It seems a good-first-issue for me. I will give it a try. 😉

[linyihai]

@rustbot claim

I will try to support it. if you have any idea, please let me know. 😁

[linyihai]

Hi, I found the code that trigger this issue.

paths:::create_dir_all_excluded_from_backups_atomic

pub fn create_dir_all_excluded_from_backups_atomic(p: impl AsRef<Path>) -> Result<()> {
    ......

    if let Err(e) = fs::rename(tempdir.path(), path) {
        if !path.exists() {
            return Err(anyhow::Error::from(e));
        }
    }
    Ok(())
}

It will failed when fs::rename want to copy the temp folder to target folder which is a broken symlink. and then it return the err without the err context like .with_context(|| format!("failed to create directory {}", p.display())).

In my opinion, the simple way is to add the err context like

    if let Err(e) = fs::rename(tempdir.path(), path) {
        if !path.exists() {
            return Err(anyhow::Error::from(e)).with_context(|| format!("failed to create directory `{}`", path.display()));
        }
    }

In addition, it is necessary to check whether the symlink is valid?

[linyihai]

Hi, I found the code that trigger this issue.

paths:::create_dir_all_excluded_from_backups_atomic

pub fn create_dir_all_excluded_from_backups_atomic(p: impl AsRef<Path>) -> Result<()> {
    ......

    if let Err(e) = fs::rename(tempdir.path(), path) {
        if !path.exists() {
            return Err(anyhow::Error::from(e));
        }
    }
    Ok(())
}

It will failed when fs::rename want to copy the temp folder to target folder which is a broken symlink. and then it return the err without the err context like .with_context(|| format!("failed to create directory {}", p.display())).

In my opinion, the simple way is to add the err context like

    if let Err(e) = fs::rename(tempdir.path(), path) {
        if !path.exists() {
            return Err(anyhow::Error::from(e)).with_context(|| format!("failed to create directory `{}`", path.display()));
        }
    }

In addition, it is necessary to check whether the symlink is valid?

@weihanglo Hi bro, what do you think?

[hi-rustin]

@linyihai Could you please give an example output to help us to check the new error message?

[weihanglo]

Hi @linyihai, that looks about right.

One more improvement you can do is using anyhow::anyhow!("failed to create directory ``{}``", path.display()) instead of return Err(….with_context(…)).

In addition, it is necessary to check whether the symlink is valid?

The above error message should be sufficient for people to find what caused the error. I don't feel like we need more checks here.

[linyihai]

@linyihai Could you please give an example output to help us to check the new error message?

make the target folder refer to broken symlink like /a/b/c, then run cargo build, the output is:

error: Not a directory (os error 20)

the detailed error message is missing.

[linyihai]

I think the final output will be like this:

cargo build
error: failed to create directory `/root/workspace/playground/target`

Caused by:
  the path `/root/workspace/playground/target` is invalid or broken symbolic link

Caused by:
  Not a directory (os error 20)

I lean to keep the initial error.

Caused by:
  Not a directory (os error 20)

[weihanglo]

Fair enough. Feel free to open a pull request.

[rben01]

@linyihai Are you sure that that

cargo/crates/cargo-util/src/paths.rs

Line 695 in a9d71d2

if let Err(e) = fs::rename(tempdir.path(), path) {

is the issue? I would assume that the create_dir_all(parent)? call above would fail before reaching that code.

[weihanglo]

create_dir_all already got a nicer error message:

cargo/crates/cargo-util/src/paths.rs

Lines 435 to 443 in a9d71d2

	pub fn create_dir_all(p: impl AsRef<Path>) -> Result<()> {
	_create_dir_all(p.as_ref())
	}
	fn _create_dir_all(p: &Path) -> Result<()> {
	fs::create_dir_all(p)
	.with_context(|| format!("failed to create directory `{}`", p.display()))?;
	Ok(())
	}