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

fix: handle broken symbolic links that were listing as missing files #164

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Psycojoker
Copy link

Hello,

This MR is to fix a hard to debug situation.

Consider this situation:

  • file "a" exists
  • symbolic links "points-to-a" exists and points to "a"
  • both the file and the symbolic links are added and committed into the code repository

Later on:

  • file "a" is removed and a commit is made
  • "check-manifest" will complains that "points-to-a" is a missing file
  • but "points-to-a" is still present and appears if a "ls" is done on it
  • therefor this can be very confusing and time consuming to debug

This MR changes the output from:

some files listed as being under source control are missing:
  points-to-a

To:

some symbolic links listed as being under source control are pointing to missing files:                                                                                                                                                                                                     
  to-qsd -> /home/psycojoker/code/python/check-manifest/qsd (missing)

I'm not sure this is the best output so feel free to modify it.

As a side note, this MR doesn't handle this situation for the "missing from sdist" analysis and it should probably be added as you can tell from this screenshot:

image

Thanks a lot for this tool ❤️

Copy link
Owner

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good. I'll want to come up with a regression test for this case, to test keep coverage at 100%.

check_manifest.py Outdated Show resolved Hide resolved
broken_symlinks = set(existing_source_files_with_broken_simlinks) - set(existing_source_files)
if broken_symlinks:
ui.warning("some symbolic links listed as being under source control are pointing to missing files:\n%s"
% format_list(["%s -> %s (missing)" % (x, os.path.realpath(x)) for x in broken_symlinks]))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, is it better to use os.path.realpath(x) here, or is it better to use os.readlink(x) here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've switch to os.readlink to try it but the result ended up being the same?

Apparently it can returns relative path sometime according to the documentation but I don't know when. I guess this will be more readable in some situations?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose the difference would be if you had two symlinks, like a -> b and b -> c. os.realpath('a') would return 'c', but os.readlink('a') would return 'b'.

But we know that in this case b doesn't exist, so it can't be a symlink to c.

How about symlinks to directories? a -> b/c, where b -> d, and d/c is missing. os.realpath('a') is 'd/c', and os.readlink('a') is 'b/c'. Which would be better to show in the error message?

To me readlink() feels a bit better.

@Psycojoker Psycojoker force-pushed the handle-broken-symbolic-links branch from 6cc1261 to fe7963c Compare June 7, 2023 23:41
@Psycojoker
Copy link
Author

Looks mostly good. I'll want to come up with a regression test for this case, to test keep coverage at 100%.

I don't know how I managed to miss the test file 😅

I've added a test but I'm not fully sure I did it correctly and in the good section :/

@Psycojoker Psycojoker force-pushed the handle-broken-symbolic-links branch from fe7963c to 8214954 Compare June 26, 2023 22:59
Copy link
Owner

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the test!

Weirdly I cannot reproduce the GitHub Actions CI failure locally (with SKIP_NO_TESTS=1 FORCE_TEST_VCS=svn tox -e py310).

For the Appveyor failures, see inline comments:

Comment on lines +1822 to +1823
os.symlink(os.path.join(self.tmpdir, "some-file"),
os.path.join(self.tmpdir, "some-symbolic-link"))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing the Appveyor CI failure on Windows, I think

Suggested change
os.symlink(os.path.join(self.tmpdir, "some-file"),
os.path.join(self.tmpdir, "some-symbolic-link"))
os.symlink(os.path.join(self.tmpdir, "some-file"), "some-symbolic-link")

might work better ...

os.unlink('some-file')
check_manifest()
self.assertIn("some symbolic links listed as being under source control are pointing to missing files:\n"
f" some-symbolic-link -> {os.path.join(self.tmpdir, 'some-file')} (missing)",
Copy link
Owner

@mgedmin mgedmin Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... because then you might be able to use

Suggested change
f" some-symbolic-link -> {os.path.join(self.tmpdir, 'some-file')} (missing)",
" some-symbolic-link -> some-file (missing)",

here.

Of course, Windows being Windows, there's a chance that even relative symlinks will be converted into those weird absolute UNC paths, in which case plan B is

# On Windows os.readlink() returns weird UNC paths so we cannot hardcode os.path.join(self.tmpdir, "...") here
link_target = os.readlink(os.path.join(self.tmpdir, "some-symbolic-link"))

followed by using

Suggested change
f" some-symbolic-link -> {os.path.join(self.tmpdir, 'some-file')} (missing)",
f" some-symbolic-link -> {link_target} (missing)",

here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants