Fix install: invalid link at destination

[cre4ture]

addresses issue #3565

Problem is that fs::copy fails when at the destination an invalid symbolic link exists.
I guess it tries to write to the file which the existing destination link points to.
Not sure if this should be even reported as an issue to rustlib itself.

The workaround I implemented is to check if the existing file is any link and remove it.
I didn't implement to check for invalid links. So all existing links will be removed before fs::copy is called.
EDIT: We simplified it by just removing all existings target files before copy.

I'm no install usecase expert, but I hope that there is no usecase where the existing link at destination needs to be used to redirect the writing of the data.

Tests are extended accordingly.

EDIT Addition: As I have now a FreeBSD VM running, I was able to make some of the disabled tests running on FreeBSD.
Issue was mainly that we need a specific binary for stripping symbols for FreeBSD and that the objdump executable doesn't exists, but rather llvm-objdump.

[tertsdiepraam]

Nice find! I think the behaviour might be even more general. Looking at the output of strace, GNU always removes a file if it already exists, regardless of whether it's a symlink.

This is the GNU strace of me copying b to the folder to if to/b exists and is a regular file:

openat(AT_FDCWD, "to", O_RDONLY|O_PATH|O_DIRECTORY) = 3
newfstatat(AT_FDCWD, "b", {st_mode=S_IFREG|0644, st_size=0, ...}, 0) = 0
newfstatat(3, "b", {st_mode=S_IFREG|0755, st_size=0, ...}, AT_SYMLINK_NOFOLLOW) = 0
unlinkat(3, "b", 0)                     = 0
openat(AT_FDCWD, "b", O_RDONLY)         = 4
newfstatat(4, "", {st_mode=S_IFREG|0644, st_size=0, ...}, AT_EMPTY_PATH) = 0
openat(3, "b", O_WRONLY|O_CREAT|O_EXCL, 0600) = 5

[cre4ture]

The documentation of fs::copy states explicitly that any file at destination will be overwritten. So its not needed for us to delete the file explicitly before. But we could remove the check if the exisiting destination is a link. This would simplify the code a bit.
But the difference is quite small. Shall I do the adaption to also remove regular files?

[tertsdiepraam]

Yeah I think that makes sense! We could still include a little comment to explain why it's necessary.

[cre4ture]

done

[github-actions]

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

[github-actions]

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

[sylvestre]

can it be simplified this way ?

Suggested change

	match fs::remove_file(to) {
	Ok(()) => {}
	Err(e) if e.kind() == std::io::ErrorKind::NotFound => { /* expected */ }
	Err(e) => {
	show_error!(
	"Failed to remove existing file {}. Error: {:?}",
	to.display(),
	e
	);
	}
	}
	if let Err(e) = fs::remove_file(to) {
	if e.kind() != std::io::ErrorKind::NotFound {
	show_error!("Failed to remove existing file {}. Error: {:?}", to.display(), e);
	}
	}

[cre4ture]

true. done. Thanks :-)

[sylvestre]

sweet, well done :)