edalize icon indicating copy to clipboard operation
edalize copied to clipboard

Command Generator Enhancements

Open GCHQDeveloper560 opened this issue 3 years ago • 4 comments

This is an initial attempt to support phony targets as discussed in #250. It seems likely that something like the Target class will be needed rather than just a string as is currently used, but it seems awkward to have self.EdaCommands.Target all over. I'm open to alternatives!

GCHQDeveloper560 avatar Jun 30 '21 16:06 GCHQDeveloper560

I took a first pass at converting Quartus to use the command generator. First, it has a case that uses multiple commands to generate a target, so I've added this to the command generator.

The Quartus Makefiles also uses variables. I thought about adding a add_variable method to the command generator, but while the Quartus case just uses simply expanded variables other backends use other types, making this less trivial. For now I've just appended variables to the header, which is likely poor form. I'm certainly open to other suggestions on how to handle this. The ninja_syntax Python API for Ninja just uses a variable method, but I also think it only has one type of variable!

GCHQDeveloper560 avatar Jul 07 '21 10:07 GCHQDeveloper560

The failing tests are unrelated to this work. See #254 for a reporting test that works with the new Pandas 1.3. I rebased this branch on top of that one and all seems well.

GCHQDeveloper560 avatar Jul 07 '21 12:07 GCHQDeveloper560

Thanks for this. I agree that it looks a bit clumsy to have self.EdaCommands.Target everywhere. Could we perhaps add a flag instead to add_command to mark the rule as generating phony targets. That won't work if the rule creates a mix of phony and regular targets but perhaps that's not a real issue. Would need to dig into that, or perhaps you know?

Regarding variables, as far as I can tell they made a deliberate choice in ninja to downplay the use of variables so that they are just simple aliases. This makes me suspect that a) we need to be careful if we want to make this portable between command executioners and b) perhaps we can get around the need for variables in most (all?) situations. The ones I'm most worried about are the cases that rely on environment variables. Come to think of it, does that even work on Windows as it is implemented today? No idea

olofk avatar Jul 26 '21 21:07 olofk

Taking another look at this, long overdue. Wouldn't it be possible to solve this by simple creating an extra target, like we do here https://github.com/olofk/edalize/blob/master/edalize/tools/icetime.py#L60

That doesn't create a phony target per se, but we could either regard all targets without a real command as phony, or if that doesn't work, add a phony plag to the add function. Would that solve your issue?

Also, do we want to pull in the first panda commit as is?

olofk avatar Jan 06 '23 22:01 olofk