Make sorting in collection_loader match plugin loader (#65776)

* Simply sorting of Windows files below other plugin types
    Using the sort method with a custom key function uses less memory than creating multiple lists then joining them.

    This seemed to be an acceptable use of a lamdba, even though I geneally try to avoid them.

* Fix sorting of plugins inside of collections
    Explicitly sort Windows files below others, mimicking what we do in plugin/loader.py

* Add documentation about ansible.builtin and ansible.legacy
    Also document to the two different methods used for searching based on the candidate type.

* Add changelog
* Add integration test
* Update comment with expected sort order

changelogs/fragments/collection_loader-sort-plugins.yaml

@@ -0,0 +1,2 @@
+bugfixes:
+  - collection_loader - sort Windows modules below other plugin types so the correct builtin plugin inside a role is selected (https://github.com/ansible/ansible/issues/65298)

lib/ansible/plugins/loader.py

@@ -271,19 +271,15 @@ class PluginLoader:
         # PLUGIN_PATHS_CACHE, and MODULE_CACHE.  Since those three dicts key
         # on the class_name and neither regular modules nor powershell modules
         # would have class_names, they would not work as written.
-        reordered_paths = []
+        #
-        win_dirs = []
+        # The expected sort order is paths in the order in 'ret' with paths ending in '/windows' at the end,
-
+        # also in the original order they were found in 'ret'.
-        for path in ret:
+        # The .sort() method is guaranteed to be stable, so original order is preserved.
-            if path.endswith('windows'):
+        ret.sort(key=lambda p: p.endswith('/windows'))
-                win_dirs.append(path)
-            else:
-                reordered_paths.append(path)
-        reordered_paths.extend(win_dirs)
 
         # cache and return the result
-        self._paths = reordered_paths
+        self._paths = ret
-        return reordered_paths
+        return ret
 
     def _load_config_defs(self, name, module, path):
         ''' Reads plugin docs to find configuration setting definitions, to push to config manager for later use '''
@@ -317,6 +313,10 @@ class PluginLoader:
                 display.debug('Added %s to loader search path' % (directory))
 
     def _find_fq_plugin(self, fq_name, extension):
+        """Search builtin paths to find a plugin. No external paths are searched,
+        meaning plugins inside roles inside collections will be ignored.
+        """
+
         plugin_type = AnsibleCollectionRef.legacy_plugin_dir_to_plugin_type(self.subdir)
 
         acr = AnsibleCollectionRef.from_fqcr(fq_name, plugin_type)
@@ -397,10 +397,12 @@ class PluginLoader:
                 try:
                     # HACK: refactor this properly
                     if candidate_name.startswith('ansible.legacy'):
-                        # just pass the raw name to the old lookup function to check in all the usual locations
+                        # 'ansible.legacy' refers to the plugin finding behavior used before collections existed.
+                        # They need to search 'library' and the various '*_plugins' directories in order to find the file.
                         full_name = name
                         p = self._find_plugin_legacy(name.replace('ansible.legacy.', '', 1), ignore_deprecated, check_aliases, suffix)
                     else:
+                        # 'ansible.builtin' should be handled here. This means only internal, or builtin, paths are searched.
                         full_name, p = self._find_fq_plugin(candidate_name, suffix)
                     if p:
                         return full_name, p
@@ -417,6 +419,9 @@ class PluginLoader:
         return name, self._find_plugin_legacy(name, ignore_deprecated, check_aliases, suffix)
 
     def _find_plugin_legacy(self, name, ignore_deprecated=False, check_aliases=False, suffix=None):
+        """Search library and various *_plugins paths in order to find the file.
+        This was behavior prior to the existence of collections.
+        """
 
         if check_aliases:
             name = self.aliases.get(name, name)

lib/ansible/utils/collection_loader.py

@@ -290,6 +290,15 @@ class AnsibleFlatMapLoader(object):
         for root, dirs, files in os.walk(root_path):
             # add all files in this dir that don't have a blacklisted extension
             flat_files.extend(((root, f) for f in files if not any((f.endswith(ext) for ext in self._extension_blacklist))))
+
+        # HACK: Put Windows modules at the end of the list. This makes collection_loader behave
+        # the same way as plugin loader, preventing '.ps1' from modules being selected before '.py'
+        # modules simply because '.ps1' files may be above '.py' files in the flat_files list.
+        #
+        # The expected sort order is paths in the order they were in 'flat_files'
+        # with paths ending in '/windows' at the end, also in the original order they were
+        # in 'flat_files'. The .sort() method is guaranteed to be stable, so original order is preserved.
+        flat_files.sort(key=lambda p: p[0].endswith('/windows'))
         self._dirtree = flat_files
 
     def find_file(self, filename):

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/testrole/tasks/main.yml

@@ -1,3 +1,9 @@
+# test using builtin module of multiple types in a role in a collection
+# https://github.com/ansible/ansible/issues/65298
+- name: Run setup module because there is both setup.ps1 and setup.py
+  setup:
+    gather_subset: min
+
 - name: check collections list from role meta
   plugin_lookup:
   register: pluginlookup_out