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 #250

Open
GCHQDeveloper560 opened this issue Jun 30, 2021 · 5 comments
Open

Command Generator Enhancements #250

GCHQDeveloper560 opened this issue Jun 30, 2021 · 5 comments

Comments

@GCHQDeveloper560
Copy link
Contributor

The new command generator added in 7ef76b0 as part of #243 doesn't support phony targets. Looking at the Vivado rework as an example, almost all of its targets including synth, syn, build-gui, and pgm aren't real files that are being generated. The old Makefile template wasn't wasn't strict about this. It used to mark pgm as phony, though this was lost in 2fce131. It didn't mark the other phony targets.

This seems unlikely to cause a real problem since a user would need to be working in the build directory and create files with these names to break the build. The fact most of Vivado's phony targets weren't properly marked even before this change would seem to support this conclusion.

However, phony targets feel like a useful concept that I believe is supported by other command executers like Ninja, so it should be more broadly applicable than Makefiles.

@GCHQDeveloper560 GCHQDeveloper560 changed the title Command Generator Doesn't Support Phony Targets Command Generator Enhancements Jul 9, 2021
@GCHQDeveloper560
Copy link
Contributor Author

As discussed in #251 the command generator should probably also support cases where multiple commands need to be run to generate a target.

I'm less sure whether variables should be supported, and if so whether or how make's multiple variables types should be supported. On one hand use of variables is a good practice, but on the other hand they're perhaps less important in machine-generated Makefiles unless they may be modified by hand later.

@olofk
Copy link
Owner

olofk commented Jul 26, 2021

Very good point about the phony targets. I want to avoid adding Makefile-specific things because, as you noted, I want to eventually support other flow executioners (first ninja, but also simpler like just as list of commands in bat/sh file and more advanced like SLURM and other cluster workload managers). Phony targets seem safe though and are a known thing at least in ninja.

Variables are more tricky. I have thought about it and it might end up inevitable to add support, but hopefully we can avoid it somehow. Since we are dealing with autogenerated makefiles I think it's a reasonable trade-off to expand variables inside Edalize compared to having the more elegant makefiles we used to have. Somewhat related to this is that we are long term aiming to support partial recompilation where only the part of the design actually changed will need to be recompiled. This will probably make a mess of the makefiles to the point where they could probably no longer be regarded as human readable

@imphil
Copy link
Collaborator

imphil commented Jul 26, 2021

I want to avoid adding Makefile-specific things because, as you noted, I want to eventually support other flow executioners (first ninja, but also simpler like just as list of commands in bat/sh file and more advanced like SLURM and other cluster workload managers).

Do you have a doc describing the ideas?

@olofk
Copy link
Owner

olofk commented Jul 26, 2021

I don't think so. This one https://github.com/olofk/edalize/wiki/Edalize-(Slight-return) only mentions ninja in passing, but not the grand vision of separating the flow from the execution of the individual tools

@imphil
Copy link
Collaborator

imphil commented Jul 26, 2021

I'm mainly wondering about the motivation to have an abstraction between edalize and ninja, or in other words, use cases that ninja wouldn't work for.

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

No branches or pull requests

3 participants