correct, cleanup & simplify dwim stack (#25956)

* correct, cleanup & simplify dwim stack

latlh chIS logh HeS qar wej chel laD
better errors
update find_file to new exception

* addressed latest comments

* test should not use realpath as it follows symlink

this fails when on OS X as /var is now a symlink to /private/var
but first_found was not supposed to follow symlinks

lib/ansible/errors/__init__.py

@@ -19,6 +19,8 @@
 from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
+from collections import Sequence
+
 from ansible.errors.yaml_strings import (
     YAML_COMMON_DICT_ERROR,
     YAML_COMMON_LEADING_TAB_ERROR,
@@ -220,7 +222,25 @@ class AnsibleUndefinedVariable(AnsibleRuntimeError):
 
 class AnsibleFileNotFound(AnsibleRuntimeError):
     ''' a file missing failure '''
-    pass
+
+    def __init__(self, message="", obj=None, show_content=True, suppress_extended_error=False, orig_exc=None, paths=None, file_name=None):
+
+        self.file_name = file_name
+        self.paths = paths
+
+        if self.file_name:
+            if message:
+                message += "\n"
+            message += "Could not find or access '%s'" % to_text(self.file_name)
+
+        if self.paths and isinstance(self.paths, Sequence):
+            searched = to_text('\n\t'.join(self.paths))
+            if message:
+                message += "\n"
+            message += "Searched in:\n\t%s" % searched
+
+        super(AnsibleFileNotFound, self).__init__(message=message, obj=obj, show_content=show_content,
+                                                  suppress_extended_error=suppress_extended_error, orig_exc=orig_exc)
 
 
 class AnsibleActionSkip(AnsibleRuntimeError):

lib/ansible/parsing/dataloader.py

@@ -22,6 +22,7 @@ __metaclass__ = type
 import copy
 import os
 import json
+import re
 import tempfile
 from yaml import YAMLError
 
@@ -42,6 +43,10 @@ except ImportError:
     from ansible.utils.display import Display
     display = Display()
 
+# Tries to determine if a path is inside a role, last dir must be 'tasks'
+# this is not perfect but people should really avoid 'tasks' dirs outside roles when using Ansible.
+RE_TASKS = re.compile(u'(?:^|%s)+tasks%s?$' % (os.path.sep, os.path.sep))
+
 
 class DataLoader:
 
@@ -184,7 +189,7 @@ class DataLoader:
 
         b_file_name = to_bytes(file_name)
         if not self.path_exists(b_file_name) or not self.is_file(b_file_name):
-            raise AnsibleFileNotFound("the file named '%s' does not exist, or is not readable" % file_name)
+            raise AnsibleFileNotFound("Unable to retrieve file contents", file_name=file_name)
 
         show_content = True
         try:
@@ -233,35 +238,33 @@ class DataLoader:
         given = unquote(given)
         given = to_text(given, errors='surrogate_or_strict')
 
-        if given.startswith(u"/"):
+        if given.startswith(to_text(os.path.sep)) or given.startswith(u'~'):
-            return os.path.abspath(given)
+            path = given
-        elif given.startswith(u"~"):
-            return os.path.abspath(os.path.expanduser(given))
         else:
             basedir = to_text(self._basedir, errors='surrogate_or_strict')
-            return os.path.abspath(os.path.join(basedir, given))
+            path = os.path.join(basedir, given)
+
+        return unfrackpath(path, follow=False)
 
     def _is_role(self, path):
-        ''' imperfect role detection, roles are still valid w/o main.yml/yaml/etc '''
+        ''' imperfect role detection, roles are still valid w/o tasks|meta/main.yml|yaml|etc '''
 
-        isit = False
         b_path = to_bytes(path, errors='surrogate_or_strict')
-        b_upath = to_bytes(unfrackpath(path), errors='surrogate_or_strict')
+        b_upath = to_bytes(unfrackpath(path, follow=False), errors='surrogate_or_strict')
 
+        for finddir in (b'meta', b'tasks'):
             for suffix in (b'.yml', b'.yaml', b''):
                 b_main = b'main%s' % (suffix)
-            b_tasked = b'tasks/%s' % (b_main)
+                b_tasked = b'%s/%s' % (finddir, b_main)
 
                 if (
-                b_path.endswith(b'tasks') and
+                    RE_TASKS.search(path) and
                     os.path.exists(os.path.join(b_path, b_main)) or
                     os.path.exists(os.path.join(b_upath, b_tasked)) or
                     os.path.exists(os.path.join(os.path.dirname(b_path), b_tasked))
                 ):
-                isit = True
+                    return True
-                break
+        return False
-
-        return isit
 
     def path_dwim_relative(self, path, dirname, source, is_role=False):
         '''
@@ -273,35 +276,35 @@ class DataLoader:
         '''
 
         search = []
+        source = to_text(source, errors='surrogate_or_strict')
 
         # I have full path, nothing else needs to be looked at
-        if source.startswith('~') or source.startswith(os.path.sep):
+        if source.startswith(to_text(os.path.sep)) or source.startswith(u'~'):
-            search.append(self.path_dwim(source))
+            search.append(unfrackpath(source, follow=False))
         else:
             # base role/play path + templates/files/vars + relative filename
             search.append(os.path.join(path, dirname, source))
-            basedir = unfrackpath(path)
+            basedir = unfrackpath(path, follow=False)
 
             # not told if role, but detect if it is a role and if so make sure you get correct base path
             if not is_role:
                 is_role = self._is_role(path)
 
-            if is_role and path.endswith('tasks'):
+            if is_role and RE_TASKS.search(path):
-                basedir = unfrackpath(os.path.dirname(path))
+                basedir = unfrackpath(os.path.dirname(path), follow=False)
 
             cur_basedir = self._basedir
             self.set_basedir(basedir)
             # resolved base role/play path + templates/files/vars + relative filename
-            search.append(self.path_dwim(os.path.join(basedir, dirname, source)))
+            search.append(unfrackpath(os.path.join(basedir, dirname, source), follow=False))
             self.set_basedir(cur_basedir)
 
             if is_role and not source.endswith(dirname):
                 # look in role's tasks dir w/o dirname
-                search.append(self.path_dwim(os.path.join(basedir, 'tasks', source)))
+                search.append(unfrackpath(os.path.join(basedir, 'tasks', source), follow=False))
 
             # try to create absolute path for loader basedir + templates/files/vars + filename
-            search.append(self.path_dwim(os.path.join(dirname, source)))
+            search.append(unfrackpath(os.path.join(dirname, source), follow=False))
-            search.append(self.path_dwim(os.path.join(basedir, source)))
 
             # try to create absolute path for loader basedir + filename
             search.append(self.path_dwim(source))
@@ -321,24 +324,25 @@ class DataLoader:
             is prepended to the source to form the path to search for.
         :arg source: A text string which is the filename to search for
         :rtype: A text string
-        :returns: An absolute path to the filename ``source``
+        :returns: An absolute path to the filename ``source`` if found
+        :raises: An AnsibleFileNotFound Exception if the file is found to exist in the search paths
         '''
         b_dirname = to_bytes(dirname)
         b_source = to_bytes(source)
 
         result = None
+        search = []
         if source is None:
             display.warning('Invalid request to find a file that matches a "null" value')
         elif source and (source.startswith('~') or source.startswith(os.path.sep)):
             # path is absolute, no relative needed, check existence and return source
-            test_path = unfrackpath(b_source)
+            test_path = unfrackpath(b_source, follow=False)
             if os.path.exists(to_bytes(test_path, errors='surrogate_or_strict')):
                 result = test_path
         else:
-            search = []
             display.debug(u'evaluation_path:\n\t%s' % '\n\t'.join(paths))
             for path in paths:
-                upath = unfrackpath(path)
+                upath = unfrackpath(path, follow=False)
                 b_upath = to_bytes(upath, errors='surrogate_or_strict')
                 b_mydir = os.path.dirname(b_upath)
 
@@ -373,6 +377,9 @@ class DataLoader:
                     result = to_text(b_candidate)
                     break
 
+        if result is None:
+            raise AnsibleFileNotFound(file_name=source, paths=search)
+
         return result
 
     def _create_content_tempfile(self, content):
@@ -401,7 +408,7 @@ class DataLoader:
 
         b_file_path = to_bytes(file_path, errors='surrogate_or_strict')
         if not self.path_exists(b_file_path) or not self.is_file(b_file_path):
-            raise AnsibleFileNotFound("the file named '%s' does not exist, or is not accessible" % to_native(file_path))
+            raise AnsibleFileNotFound(file_name=file_path)
 
         if not self._vault:
             self._vault = VaultLib(b_password="")
@@ -420,7 +427,7 @@ class DataLoader:
                         # since the decrypt function doesn't know the file name
                         data = f.read()
                         if not self._b_vault_password:
-                            raise AnsibleParserError("A vault password must be specified to decrypt %s" % file_path)
+                            raise AnsibleParserError("A vault password must be specified to decrypt %s" % to_native(file_path))
 
                         data = self._vault.decrypt(data, filename=real_path)
                         # Make a temp file

lib/ansible/plugins/action/__init__.py

@@ -981,9 +981,5 @@ class ActionBase(with_metaclass(ABCMeta, object)):
         # dwim already deals with playbook basedirs
         path_stack = self._task.get_search_path()
 
-        result = self._loader.path_dwim_relative_stack(path_stack, dirname, needle)
+        # if missing it will return a file not found exception
-
+        return self._loader.path_dwim_relative_stack(path_stack, dirname, needle)
-        if result is None:
-            raise AnsibleError("Unable to find '%s' in expected paths." % to_native(needle))
-
-        return result

lib/ansible/plugins/lookup/__init__.py

@@ -22,6 +22,7 @@ __metaclass__ = type
 from abc import ABCMeta, abstractmethod
 
 from ansible.module_utils.six import with_metaclass
+from ansible.errors import AnsibleFileNotFound
 
 try:
     from __main__ import display
@@ -112,8 +113,11 @@ class LookupBase(with_metaclass(ABCMeta, object)):
         else:
             paths = self.get_basedir(myvars)
 
+        result = None
+        try:
             result = self._loader.path_dwim_relative_stack(paths, subdir, needle)
-        if result is None and not ignore_missing:
+        except AnsibleFileNotFound:
+            if not ignore_missing:
                 self._display.warning("Unable to find '%s' in expected paths." % needle)
 
         return result

lib/ansible/vars/manager.py

@@ -369,11 +369,13 @@ class VariableManager:
                         raise AnsibleUndefinedVariable("an undefined variable was found when attempting to template the vars_files item '%s'" % vars_file_item,
                                                        obj=vars_file_item)
                     else:
-                        # we do not have a full context here, and the missing variable could be
+                        # we do not have a full context here, and the missing variable could be because of that
-                        # because of that, so just show a warning and continue
+                        # so just show a warning and continue
                         display.vvv("skipping vars_file '%s' due to an undefined variable" % vars_file_item)
                         continue
 
+                display.vvv("Read vars_file '%s'" % vars_file_item)
+
             # By default, we now merge in all vars from all roles in the play,
             # unless the user has disabled this via a config option
             if not C.DEFAULT_PRIVATE_ROLE_VARS:

test/integration/targets/iterators/tasks/main.yml

@@ -216,10 +216,10 @@
     - "{{ output_dir + '/bar1' }}"
 
 - name: set expected
-  set_fact: first_expected="{{ output_dir | expanduser | realpath + '/foo1' }}"
+  set_fact: first_expected="{{ output_dir | expanduser + '/foo1' }}"
 
 - name: set unexpected
-  set_fact: first_unexpected="{{ output_dir | expanduser | realpath + '/bar1' }}"
+  set_fact: first_unexpected="{{ output_dir | expanduser + '/bar1' }}"
 
 - name: verify with_first_found results
   assert:

test/integration/targets/win_copy/tasks/main.yml

@@ -329,7 +329,7 @@
     src: false-file
     dest: "{{win_output_dir}}\\fale-file.txt"
   register: fail_missing_source
-  failed_when: fail_missing_source.msg != "Unable to find 'false-file' in expected paths."
+  failed_when: not (fail_missing_source|failed)
 
 - name: fail when copying remote src file when src doesn't exist
   win_copy: