Repository: olofk/edalize

Issue:
QSys Pro 23 files are not processed correctly by the filter
I've recently had to create some new QSys files, and was surprised to see the build failed because the IP didn't get generated correctly. This was a bit of a surprised, as an existing project withb QSys in it still worked fine with the latest FuseSoC and Edalize.

On investigation, it seems that for v23, they've completely changed the format of the XML that QSys generates. After tracing through the code, it's become clear that the check inside qsys_file_filter in quartus.py is now failing to detect that this is a QSys Pro file, and so not running generation on it. To make life more complicated, they still call it version 1.0, so detecting it can't be a simple version check inside the file.

I think it should be fine to check both variants. I'll try some local changes, and if they work will put up a PR to work around this file format change. I see the fallback just assumed Standard rather than Pro - I suspect changing this to assume the same as the version of Quartus installed might be a better choice, so I might do this at the same time, unless anyone objects.

Hints from issue comments:
@GCHQDeveloper137: Aha, the fallback comment states that we should treat it as standard, but looking at the code it actually gets treated as the /opposite/ of the variant of the tool in use. That's clearly a very strange thing to do - if running standard the template will ignore this, and if running pro it would be filtered out so would ignore it! I'll make it default to the version in use, and update the comment to match the code.@GCHQDeveloper137: Aha, the old behaviour was correct - we're matching versions up in here, my mistake. I'll just update the comments to be a little clearer.@olofk: EDA tool vendors never cease to amaze me when it comes to finding new ways to create compatibility with the world around them, including their own tools.

As I don't have access to Quartus Pro myself, a PR would be most welcome if there is a fix needed.@GCHQDeveloper137: I've got something that works fine for all the tool versions I have access to (old Standard, old Pro and new Pro). Thankfully, after updating the tests it spotted an issue with what you'd expect the new format Standard to look like, so I've fixed that. PR incoming shortly.

Solution patch:
diff --git a/edalize/quartus.py b/edalize/quartus.py
index f6068ad..2f8d723 100644
--- a/edalize/quartus.py
+++ b/edalize/quartus.py
@@ -184,13 +184,20 @@ class Quartus(Edatool):
             try:
                 qsysTree = ET.parse(os.path.join(self.work_root, f.name))
                 try:
-                    tool = qsysTree.find("component").attrib["tool"]
-                    if tool == "QsysPro" and self.isPro:
+                    try:
+                        tool = qsysTree.find("component").attrib["tool"]
+                    except AttributeError:
+                        tool = qsysTree.find(".//{http://www.altera.com/XMLSchema/IPXact2014/extensions}tool").text
+                    if tool == "QsysPro":
+                        if self.isPro:
+                            name = f.name
+                    elif not self.isPro:
                         name = f.name
                 except (AttributeError, KeyError):
                     # Either a component wasn't found in the QSYS file, or it
                     # had no associated tool information. Make the assumption
-                    # it was a Standard edition file
+                    # it is a Standard edition file, as the old formats just
+                    # don't specify.
                     if not self.isPro:
                         name = f.name
             except (ET.ParseError, IOError):


Test patch:
diff --git a/tests/test_quartus.py b/tests/test_quartus.py
index 6f34c3b..861b002 100644
--- a/tests/test_quartus.py
+++ b/tests/test_quartus.py
@@ -10,12 +10,31 @@ qsys_file = """<?xml version="1.0" encoding="UTF-8"?>
    description=""
    tags=""
    categories="System"
-   {}
+   tool="{}"
    />
 </system>
 """
 
-qsys_fill = {"Standard": "", "Pro": 'tool="QsysPro"'}
+qsys23_file = """<?xml version="1.0" ?>
+<ipxact:design xmlns:altera="http://www.altera.com/XMLSchema/IPXact2014/extensions" xmlns:ipxact="http://www.accellera.org/XMLSchema/IPXACT/1685-2014">
+  <ipxact:vendor>Altera Corporation</ipxact:vendor>
+  <ipxact:library>sdm_mailbox</ipxact:library>
+  <ipxact:name>sdm_mailbox</ipxact:name>
+  <ipxact:version>1.0</ipxact:version>
+  <ipxact:componentInstances></ipxact:componentInstances>
+  <ipxact:vendorExtensions>
+    <altera:catalog_card_info>
+      <altera:version>1.0</altera:version>
+      <altera:description></altera:description>
+      <altera:tags></altera:tags>
+      <altera:categories></altera:categories>
+      <altera:tool>{}</altera:tool>
+    </altera:catalog_card_info>
+  </ipxact:vendorExtensions>
+</ipxact:design>
+"""
+
+qsys_fill = {"Standard": "", "Pro": "QsysPro"}
 
 test_sets = {
     "Standard": {
@@ -64,30 +83,31 @@ def test_quartus(make_edalize_test):
     # Test each edition of Quartus
     for edition in ["Standard", "Pro"]:
         for pnr in ["Quartus", "DSE"]:
-            # Each edition and P&R tool has its own set of representative files
-            if pnr == "DSE":
-                _tool_options = {**tool_options, "pnr": "dse"}
-            else:
-                _tool_options = {**tool_options}
+            for qsys_format in [qsys_file, qsys23_file]:
+                # Each edition and P&R tool has its own set of representative files
+                if pnr == "DSE":
+                    _tool_options = {**tool_options, "pnr": "dse"}
+                else:
+                    _tool_options = {**tool_options}
 
-            # Ensure we test the edition we intend to, even if quartus_sh is
-            # present
-            os.environ["FUSESOC_QUARTUS_EDITION"] = edition
+                # Ensure we test the edition we intend to, even if quartus_sh is
+                # not present
+                os.environ["FUSESOC_QUARTUS_EDITION"] = edition
 
-            tf = make_edalize_test(
-                "quartus",
-                param_types=["vlogdefine", "vlogparam"],
-                tool_options=_tool_options,
-                ref_dir=edition,
-            )
+                tf = make_edalize_test(
+                    "quartus",
+                    param_types=["vlogdefine", "vlogparam"],
+                    tool_options=_tool_options,
+                    ref_dir=edition,
+                )
 
-            # Each edition performs checks on the QSYS files present, so
-            # provide a minimal example
-            with open(os.path.join(tf.work_root, "qsys_file"), "w") as f:
-                f.write(qsys_file.format(qsys_fill[edition]))
+                # Each edition performs checks on the QSYS files present, so
+                # provide a minimal example
+                with open(os.path.join(tf.work_root, "qsys_file"), "w") as f:
+                    f.write(qsys_format.format(qsys_fill[edition]))
 
-            tf.backend.configure()
-            tf.compare_files(["Makefile", tf.test_name + ".tcl"])
+                tf.backend.configure()
+                tf.compare_files(["Makefile", tf.test_name + ".tcl"])
 
-            tf.backend.build()
-            tf.compare_files(test_sets[edition][pnr])
+                tf.backend.build()
+                tf.compare_files(test_sets[edition][pnr])