Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Command Generator Enhancements #251

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

GCHQDeveloper560
Copy link
Contributor

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 GCHQDeveloper560 changed the title Support phony targets in command generator Command Generator Enhancements Jul 7, 2021
@GCHQDeveloper560
Copy link
Contributor Author

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!

The previous syntax for checking a column of identical values fails with
Pandas 1.3. The new check works with 1.3 and older versions.
@GCHQDeveloper560
Copy link
Contributor Author

GCHQDeveloper560 commented Jul 7, 2021

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.

This passes the tests, though the Jinja filtering previously used
was complex enough that problems may appear in use.

This attempts to duplicate the previous Makefiles, though that may not
be good style with the command generator. This required modifying the
header to include Makefile variables since there's not yet support for
variables in the class
@olofk
Copy link
Owner

olofk commented Jul 26, 2021

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
Copy link
Owner

olofk commented Jan 6, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants