Fix messed up printing of shed_tool_conf.xml file

This drops xml_to_string from lib/tool_shed/util/xml_util.py
and uses xml_to_string form lib/galaxy/utils/__init__.py.
Also reduces plain string manipulation of xml structures.
Uses xml.dom.minidom to make xml pretty if necessary.

lib/galaxy/tools/data/__init__.py

@@ -15,6 +15,7 @@ import string
 import time
 from glob import glob
 from tempfile import NamedTemporaryFile
+from xml.etree import ElementTree
 
 import requests
 
@@ -185,12 +186,12 @@ class ToolDataTableManager(object):
         # add new elems
         out_elems.extend(new_elems)
         out_path_is_new = not os.path.exists(full_path)
+
+        root = ElementTree.fromstring('<?xml version="1.0"?>\n<tables></tables>')
+        for elem in out_elems:
+            root.append(elem)
         with RenamedTemporaryFile(full_path, mode='w') as out:
-            out.write('<?xml version="1.0"?>\n<tables>\n')
-            for elem in out_elems:
-                elem = util.xml_to_string(elem, pretty=True)
-                out.write(elem)
-            out.write('</tables>\n')
+            out.write(util.xml_to_string(root, pretty=True))
         os.chmod(full_path, 0o644)
         if out_path_is_new:
             self.tool_data_path_files.update_files()

lib/galaxy/util/__init__.py

@@ -22,6 +22,7 @@ import tempfile
 import threading
 import time
 import unicodedata
+import xml.dom.minidom
 from datetime import datetime
 from hashlib import md5
 from os.path import relpath
@@ -235,19 +236,14 @@ def parse_xml_string(xml_string):
 
 def xml_to_string(elem, pretty=False):
     """Returns a string from an xml tree"""
+    if PY2:
+        xml_str = ElementTree.tostring(elem)
+    else:
+        xml_str = ElementTree.tostring(elem, encoding='unicode')
     if pretty:
-        elem = pretty_print_xml(elem)
-    try:
-        if PY2:
-            return ElementTree.tostring(elem)
-        else:
-            return ElementTree.tostring(elem, encoding='unicode')
-    except TypeError as e:
-        # we assume this is a comment
-        if hasattr(elem, 'text'):
-            return "<!-- %s -->\n" % (elem.text)
-        else:
-            raise e
+        pretty_string = xml.dom.minidom.parseString(xml_str).toprettyxml(indent='    ')
+        return "\n".join([line for line in pretty_string.split('\n') if not re.match(r'^[\s\\nb\']*$', line)])
+    return xml_str
 
 
 def xml_element_compare(elem1, elem2):

lib/tool_shed/capsule/capsule_manager.py

@@ -92,7 +92,7 @@ class ExportRepositoryManager(object):
             # Keep information about the export in a file named export_info.xml in the archive.
             sub_elements = self.generate_export_elem()
             export_elem = xml_util.create_element('export_info', attributes=None, sub_elements=sub_elements)
-            tmp_export_info = xml_util.create_and_write_tmp_file(export_elem, use_indent=True)
+            tmp_export_info = xml_util.create_and_write_tmp_file(export_elem)
             try:
                 repositories_archive.add(tmp_export_info, arcname='export_info.xml')
             finally:
@@ -102,7 +102,7 @@ class ExportRepositoryManager(object):
             exported_repository_root = xml_util.create_element('repositories')
             for exported_repository_elem in exported_repository_registry.exported_repository_elems:
                 exported_repository_root.append(exported_repository_elem)
-            tmp_manifest = xml_util.create_and_write_tmp_file(exported_repository_root, use_indent=True)
+            tmp_manifest = xml_util.create_and_write_tmp_file(exported_repository_root)
             try:
                 repositories_archive.add(tmp_manifest, arcname='manifest.xml')
             finally:
@@ -165,7 +165,7 @@ class ExportRepositoryManager(object):
                         if error_message:
                             return None, error_message
                         if altered:
-                            tmp_filename = xml_util.create_and_write_tmp_file(root_elem, use_indent=True)
+                            tmp_filename = xml_util.create_and_write_tmp_file(root_elem)
                             shutil.move(tmp_filename, full_path)
                     elif name == rt_util.TOOL_DEPENDENCY_DEFINITION_FILENAME:
                         # Eliminate the toolshed, and changeset_revision attributes from all <repository> tags.
@@ -173,7 +173,7 @@ class ExportRepositoryManager(object):
                         if error_message:
                             return None, error_message
                         if altered:
-                            tmp_filename = xml_util.create_and_write_tmp_file(root_elem, use_indent=True)
+                            tmp_filename = xml_util.create_and_write_tmp_file(root_elem)
                             shutil.move(tmp_filename, full_path)
                     repository_archive.add(full_path, arcname=relative_path)
         repository_archive.close()

lib/tool_shed/galaxy_install/tools/data_manager.py

@@ -1,8 +1,9 @@
 import logging
 import os
-import threading
 import time
+from xml.etree import ElementTree
 
+from galaxy.util import xml_to_string
 from galaxy.util.renamed_temporary_file import RenamedTemporaryFile
 from tool_shed.galaxy_install.tools import tool_panel_manager
 from tool_shed.util import xml_util
@@ -29,21 +30,18 @@ class DataManagerHandler(object):
         of config_filename.
         """
         data_managers_path = self.data_managers_path
-        lock = threading.Lock()
-        lock.acquire(True)
+        if data_managers_path:
+            root_str = '<?xml version="1.0"?><data_managers tool_path="%s"></data_managers>' % data_managers_path
+        else:
+            root_str = '<?xml version="1.0"?><data_managers></data_managers>'
+        root = ElementTree.fromstring(root_str)
+        for elem in config_elems:
+            root.append(elem)
         try:
             with RenamedTemporaryFile(config_filename, mode='w') as fh:
-                if data_managers_path is not None:
-                    fh.write('<?xml version="1.0"?>\n<data_managers tool_path="%s">\n    ' % data_managers_path)
-                else:
-                    fh.write('<?xml version="1.0"?>\n<data_managers>\n    ')
-                for elem in config_elems:
-                    fh.write(xml_util.xml_to_string(elem))
-                fh.write('</data_managers>\n')
-        except Exception:
-            log.exception("Exception in DataManagerHandler.data_manager_config_elems_to_xml_file")
-        finally:
-            lock.release()
+                fh.write(xml_to_string(root))
+        except Exception as e:
+            log.exception("Exception in DataManagerHandler.data_manager_config_elems_to_xml_file:\n %s", str(e))
 
     def install_data_managers(self, shed_data_manager_conf_filename, metadata_dict, shed_config_dict,
                               relative_install_dir, repository, repository_tools_tups):
@@ -74,7 +72,7 @@ class DataManagerHandler(object):
                     data_manager_id = elem.get('id', None)
                     if data_manager_id is None:
                         log.error("A data manager was defined that does not have an id and will not be installed:\n%s" %
-                                  xml_util.xml_to_string(elem))
+                                  xml_to_string(elem))
                         continue
                     data_manager_dict = metadata_dict['data_manager'].get('data_managers', {}).get(data_manager_id, None)
                     if data_manager_dict is None:
@@ -120,7 +118,7 @@ class DataManagerHandler(object):
                     if data_manager:
                         rval.append(data_manager)
                 else:
-                    log.warning("Encountered unexpected element '%s':\n%s" % (elem.tag, xml_util.xml_to_string(elem)))
+                    log.warning("Encountered unexpected element '%s':\n%s" % (elem.tag, xml_to_string(elem)))
                 config_elems.append(elem)
                 data_manager_config_has_changes = True
             # Persist the altered shed_data_manager_config file.

lib/tool_shed/galaxy_install/tools/tool_panel_manager.py

@@ -1,7 +1,8 @@
 import logging
-import threading
-from xml.etree import cElementTree as XmlET
+from xml.etree import ElementTree as XmlET
 
+from galaxy.util import xml_to_string
+from galaxy.util.renamed_temporary_file import RenamedTemporaryFile
 from tool_shed.util import basic_util
 from tool_shed.util import common_util
 from tool_shed.util import repository_util
@@ -79,19 +80,14 @@ class ToolPanelManager(object):
         Persist the current in-memory list of config_elems to a file named by the
         value of config_filename.
         """
-        lock = threading.Lock()
-        lock.acquire(True)
         try:
-            fh = open(config_filename, 'w')
-            fh.write('<?xml version="1.0"?>\n<toolbox tool_path="%s">\n' % str(tool_path))
+            root = XmlET.fromstring('<?xml version="1.0"?>\n<toolbox tool_path="%s"></toolbox>' % str(tool_path))
             for elem in config_elems:
-                fh.write(xml_util.xml_to_string(elem, use_indent=True))
-            fh.write('</toolbox>\n')
-            fh.close()
-        except Exception:
-            log.exception("Exception in ToolPanelManager.config_elems_to_xml_file")
-        finally:
-            lock.release()
+                root.append(elem)
+            with RenamedTemporaryFile(config_filename, mode='w') as fh:
+                fh.write(xml_to_string(root, pretty=True))
+        except Exception as e:
+            log.exception("Exception in ToolPanelManager.config_elems_to_xml_file: \n %s", str(e))
 
     def generate_tool_elem(self, tool_shed, repository_name, changeset_revision, owner, tool_file_path,
                            tool, tool_section):

lib/tool_shed/metadata/metadata_generator.py

@@ -886,7 +886,7 @@ class MetadataGenerator(object):
                     # installation from proceeding.  Reaching here implies a bug in the Tool Shed
                     # framework.
                     error_message = 'Installation encountered an invalid repository dependency definition:\n'
-                    error_message += xml_util.xml_to_string(repository_elem, use_indent=True)
+                    error_message += util.xml_to_string(repository_elem, pretty=True)
                     log.error(error_message)
                     return repository_dependency_tup, False, error_message
         if not toolshed:

lib/tool_shed/util/xml_util.py

@@ -2,10 +2,12 @@ import logging
 import os
 import sys
 import tempfile
-import xml.etree.ElementTree
 from xml.etree import ElementTree as XmlET
 
-from galaxy.util import listify
+from galaxy.util import (
+    listify,
+    xml_to_string
+)
 
 log = logging.getLogger(__name__)
 using_python_27 = sys.version_info[:2] >= (2, 7)
@@ -20,17 +22,16 @@ class Py27CommentedTreeBuilder(XmlET.TreeBuilder):
         self.end(XmlET.Comment)
 
 
-def create_and_write_tmp_file(elems, use_indent=False):
+def create_and_write_tmp_file(elems):
     tmp_str = ''
     for elem in listify(elems):
-        tmp_str += xml_to_string(elem, use_indent=use_indent)
+        tmp_str += xml_to_string(elem, pretty=True)
     fh = tempfile.NamedTemporaryFile('wb', prefix="tmp-toolshed-cawrf")
     tmp_filename = fh.name
     fh.close()
-    fh = open(tmp_filename, 'wb')
-    fh.write('<?xml version="1.0"?>\n')
-    fh.write(tmp_str)
-    fh.close()
+    with open(tmp_filename, 'w') as fh:
+        fh.write('<?xml version="1.0"?>\n')
+        fh.write(tmp_str)
     return tmp_filename
 
 
@@ -78,68 +79,25 @@ def create_element(tag, attributes=None, sub_elements=None):
     return None
 
 
-def indent(elem, level=0):
-    """
-    Prints an XML tree with each node indented according to its depth.  This method is used to print the
-    shed tool config (e.g., shed_tool_conf.xml from the in-memory list of config_elems because each config_elem
-    in the list may be a hierarchical structure that was not created using the parse_xml() method below,
-    and so will not be properly written with xml.etree.ElementTree.tostring() without manually indenting
-    the tree first.
-    """
-    i = "\n" + level * "    "
-    if len(elem):
-        if not elem.text or not elem.text.strip():
-            elem.text = i + "  "
-        if not elem.tail or not elem.tail.strip():
-            elem.tail = i
-        for child in elem:
-            indent(child, level + 1)
-        if not child.tail or not child.tail.strip():
-            child.tail = i
-        if not elem.tail or not elem.tail.strip():
-            elem.tail = i
-    else:
-        if level and (not elem.tail or not elem.tail.strip()):
-            elem.tail = i
-
-
 def parse_xml(file_name):
     """Returns a parsed xml tree with comments intact."""
     error_message = ''
     if not os.path.exists(file_name):
         return None, "File does not exist %s" % str(file_name)
 
-    fobj = open(file_name, 'r')
-    if using_python_27:
-        try:
-            tree = XmlET.parse(fobj, parser=XmlET.XMLParser(target=Py27CommentedTreeBuilder()))
-        except Exception as e:
-            fobj.close()
-            error_message = "Exception attempting to parse %s: %s" % (str(file_name), str(e))
-            log.exception(error_message)
-            return None, error_message
-    else:
-        try:
-            tree = XmlET.parse(fobj)
-        except Exception as e:
-            fobj.close()
-            error_message = "Exception attempting to parse %s: %s" % (str(file_name), str(e))
-            log.exception(error_message)
-            return None, error_message
-    fobj.close()
-    return tree, error_message
-
-
-def xml_to_string(elem, encoding='utf-8', use_indent=False, level=0):
-    if elem is not None:
-        if use_indent:
-            # We were called from ToolPanelManager.config_elems_to_xml_file(), so
-            # set the level to 1 since level 0 is the <toolbox> tag set.
-            indent(elem, level=level)
+    with open(file_name, 'r') as fobj:
         if using_python_27:
-            xml_str = '%s\n' % xml.etree.ElementTree.tostring(elem, encoding=encoding, method="xml")
+            try:
+                tree = XmlET.parse(fobj, parser=XmlET.XMLParser(target=Py27CommentedTreeBuilder()))
+            except Exception as e:
+                error_message = "Exception attempting to parse %s: %s" % (str(file_name), str(e))
+                log.exception(error_message)
+                return None, error_message
         else:
-            xml_str = '%s\n' % xml.etree.ElementTree.tostring(elem, encoding=encoding)
-    else:
-        xml_str = ''
-    return xml_str
+            try:
+                tree = XmlET.parse(fobj)
+            except Exception as e:
+                error_message = "Exception attempting to parse %s: %s" % (str(file_name), str(e))
+                log.exception(error_message)
+                return None, error_message
+    return tree, error_message