Add more informative error message to setup.py install symlink error

[mrmagooey]

SUMMARY

Currently when symlinks are missing from the cloned repository, the setup.py install process raises:

[Errno 2] No such file or directory: 'SYMLINK_CACHE.json'

Which is not really indicative of the underlying problem of missing symlinks.

This PR adds a more descriptive Exception to be raised.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

setup.py

ANSIBLE VERSION

ansible 2.8.0.dev0
  config file = None
  configured module search path = ['/Users/user/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /private/tmp/venv/test/lib/python3.7/site-packages/ansible-2.8.0.dev0-py3.7.egg/ansible
  executable location = /private/tmp/venv/test/bin/ansible
  python version = 3.7.0 (default, Sep  4 2018, 12:11:50) [Clang 9.1.0 (clang-902.0.39.2)]

ADDITIONAL INFORMATION

Problem arose when cloning on a Windows machine, which silently clobbered all the symlinks, and then moving to a *nix machine and installing into a fresh virtualenv.

With a broken ansible-playbook, without PR change:

$ python setup.py install
running install
running bdist_egg
...
running install_lib
running build_py
error: [Errno 2] No such file or directory: 'SYMLINK_CACHE.json'

After change:

$ python setup.py install
running install
running bdist_egg
...
RuntimeError: Symlinks in ./bin appear to be missing or broken

[webknjaz]

I like the intent! Although I don't like introducing a new unnecessary variable and moving the block of code out of where it needs to be.
If you wish to move it, you need to move it in a function called smth like _cache_symlinks_or_die and call it in the right place, variable is not needed.

[webknjaz]

This doesn't need to be extracted out of try/except. You could've just changed only raise statement.

[mrmagooey]

Happy to either create a new function, or just call the new RuntimeException in-place.

The new variable was to avoid Python3 complaining During handling of the above exception, another exception occurred:, but if that is not an issue then I will amend this PR to just raise the new, more descriptive RuntimeException in place of where the existing Exception was being re-raised.

[abadger]

The verbosity is generally a feature although perhaps not for a user eerir.

Who is this message targeted at? It might not be very informative unless there person it's targeted at already knows that symlinks should exist and for what reason

[mrmagooey]

I'm happy to change the exception error string to something more informative as well, I guess the only argument I would make is to change it from [Errno 2] No such file or directory: 'SYMLINK_CACHE.json'.

As a potential target user for this message (i.e. someone who will be occasionally installing from source in a mixed OS environment), something like "The symlinks in ./bin appear to be broken, consider refreshing this repository from a known good source" is one potential error message.

[webknjaz]

Python3 complaining

It's not complaining, it's a feature, which allows tracking exception chain. It's actually helpful to know what caused what. (https://www.python.org/dev/peps/pep-3134/#explicit-exception-chaining, https://docs.python.org/3/library/exceptions.html)

So I would insist on preserving correct exception metadata rather than trying to confuse it.

wrt to displaying the exception to user, I'd make some kind of wrapper around _maintain_symlinks() calls. For example, decorate run methods of commands or better wrap setup() call in main() with try/except to display an error message on RuntimeError. But this definatellly should go to a separate layer: such kind of error handling doesn't belong to low-level function, but to representation layer.

[mrmagooey]

Ok, some good points, I am a little unsure as to where to take this PR though.

Currently the plan seems to be to refactor setup.py to have run handlers which will incorporate a representation layer for user error messages. This feels like a broader piece of refactoring that is outside the scope of simply changing the Exception on missing symlinks, and if that is the ultimate plan then this PR might be closed and when this representation layer is created then additional user errors could be incorporated into it.

About confusing the exception metadata, it seems to me that the missing symlink file exception will get raised for every first time setup.py install, because that file never exists in the bare repo, it just happens to be that normally the catch takes care of it. Unless I'm missing something (which I will admit is very possible) this is just a EAFP pattern, in other languages it would just be an if statement checking for the existence of the file, it just happens to be that in Python the pattern is to use Exceptions.

Keeping the file missing exception information using the chained exception doesn't seem to help for either the developer or the user. For the developer it tells them that this is a bare repository which they should probably already know, for the user it opens up a world of questions as to why this file doesn't exist. The initial exception doesn't provide the root of the problem (i.e. missing symlinks), so keeping it chained as the "cause" of the problem seems misleading. Yes the file does not exist, but also, yes that is completely normal for a first time installation, in fact we 100% expect it not to exist (for the first install run).

[webknjaz]

It's a bit hard to reason about code in comments detached from lines in diff. I'll make a few changes and then we'll be able to discuss this in "Files changes" tab.

Not relevant anymore

[mrmagooey]

Looks great, sorry for my lack of understanding of the representation layer. Is there anything further you would like me to do? I'm happy for it to be squashed and merged.

[webknjaz]

No, it's fine :) I'll merge this!

[webknjaz]

I've decided to wait on @abadger because I need to hear his opinion on using ANSIBLE_DEBUG env var. So postponing merge a bit.

[abadger]

Maybe: "Pregenerated symlink list was not present and expected symlinks in ./bin were missing or broken. Perhaps this isn't a git checkout?"

[webknjaz]

If @mrmagooey is not opposed to it, it's ok.

[mrmagooey]

I'm happy for that to go in, only thought is that the original issue (for me at least) was related to a git checkout on windows, so the final sentence might be a bit of red herring. But overall I think this is a great improvement.

[webknjaz]

Okay, let's do this.