astroid icon indicating copy to clipboard operation
astroid copied to clipboard

Resolving symlinks considered harmful

Open smurfix opened this issue 2 years ago • 4 comments

Steps to reproduce

  1. Get https://github.com/M-o-a-T/moat
  2. git submodule update --init util micro
  3. cd micro
  4. pylint

Current behavior

The problem which this archive exhibits is that moat/micro/link.py is a symlink that refers to moat/micro/_embed/lib/moat/micro/link.py. The directory moat/micro has an _init__.py file while moat/micro/_embed/lib/moat/micro does not.

Now, modpath_from_file_with_callback, in astroid/mdutils.py, calls astroid's _get_relative_base_path function which calls realpath and thus resolves this symlink. As a result, pylint's "is this a proper module" callback looks for the __init__.py file in the wrong place, decides that this is not a module, and refuses to process the import.

Expected behavior

IMHO there's no good reason to call realpath on anything. Why not simply assume that the symlink is there for a good reason?

python -c "from astroid import __pkginfo__; print(__pkginfo__.version)" output

2.14.2, but applies to master

smurfix avatar Apr 19 '23 14:04 smurfix

Did you search for the reason in the git history ? It feel like a Chesterton's fence (we should not remove it until we know why it's here in the first place)

Pierre-Sassoulas avatar Apr 23 '23 06:04 Pierre-Sassoulas

Both absolute and real path lookups were added in cd76b495d

modutils is using realpath at the moment which will resolve the symlink
and don't allow the import as the file is detected outside of the
available paths.

This change added two tests – which incidentally still work after removing that realpath call.

smurfix avatar Apr 24 '23 09:04 smurfix

Both absolute and real path lookups were added in cd76b49

modutils is using realpath at the moment which will resolve the symlink
and don't allow the import as the file is detected outside of the
available paths.

This change added two tests – which incidentally still work after removing that realpath call.

As you can see from the diff we used realpath before it. I added a comment in your PR with the first introduction of realpath, which sadly doesn't include a test.

DanielNoord avatar Apr 24 '23 09:04 DanielNoord

Ah, yes, sorry, the real reason realpath was added seems to be PyCQA/pylint#791

I'll ask there whether somebody can test this.

smurfix avatar Apr 24 '23 10:04 smurfix