Fix ordering issues in Manifest files (#2325, #2334)

[martijnbastiaan]

Fixes #2325

Fixes #2334

Still TODO:

• Write a changelog entry (see changelog/README.md)
• Check copyright notices are up to date in edited files
• Write tests
• Update Vivado TCL - it doesn't need to call update_compile_order anymore.

[DigitalBrains1]

Thanks so much for fixing this!

I wonder if there is still some way to get files wrongly sorted. I had a whole manifesto in the commit msg for my attempt:

We don't track dependency information for include files and memory
files. Include files are generated from
`Clash.Netlist.BlackBox.Types.bbIncludes` or YAML/JSON `includes` keys,
and memory files are generated from `~TEMPLATE` holes in the primitive
template language. We should actually track these dependencies to
maintain a proper reverse topological sort order in our manifest files.
However, this commit makes the problem less severe. We used to list
these include and memory files last in the list, but it is extremely
likely that the HDL file that caused the include/memory file to be
generated depends on it. So by listing them first instead of last, we
fix the sort order for the common case that the include/memory files
_themselves_ do not depend on anything else.

To be precise, we sort the include files after the memory files because
it is probably difficult to impossible to construct a memory file that
depends on an include file, but the reverse is easily achieved.

What could happen, I think, is that a primitive can reference another HDL file (because it refers to something that is generated in a separate HDL file). And if that primitive generates an include file that also references that HDL file, you'd want the include file to be in between the two HDL files in the dependency sort.

So maybe we still need a little warning somewhere that not every conceivable dependency is sorted, because that would require us to group HDL files and include files as they come to life rather than first listing all include files.

I had this FIXME in clash-lib/src/Clash/Driver.hs line 483:

-- FIXME: We should track dependencies of `mfiles` and `dfiles` and
-- maintain the proper topological sort of all these.

[DigitalBrains1]

Now that update_compile_order is gone, you could try to enable all the tests that currently says "Vivado segfaults" to see if that is still the case. Because a different compile order might actually avoid whatever dumb thing Vivado is trying to do when it segfaults.

Ah, also: you can re-enable the "Parameters" test (tests/Main.hs line 365 and further), because that one was plagued by update_compile_order being crappy. This PR closes issue #2266 !

[martijnbastiaan]

Ah, also: you can re-enable the "Parameters" test (tests/Main.hs line 365 and further), because that one was plagued by update_compile_order being crappy. This PR closes issue #2266 !

Done, that works now. The others still segfault unfortunately.

[martijnbastiaan]

Also, manifesto added to the relevant commit.

[DigitalBrains1]

This comment also needs to go

[DigitalBrains1]

Wow I thought CI would take a while, it was done in under 32 minutes?

[martijnbastiaan]

ah flip, you're right.

[martijnbastiaan]

12 jobs for
fix-2325-2334
in 18 minutes and 50 seconds

GitLab :-). GHA usually takes longer, but it was feeling generous today!