fusesoc
fusesoc copied to clipboard
Improved tool options parsing
This PR handles two issues:
- A use flag inside a tool option meant the entry was passed to the tool verbatim. This PR turns the bare string into a StringWithUseFlags and parses it with the flags
- An environment variable inside a tool option meant the entry would not parse. This PR does
os.path.expandvars()to expand any environment variables
First fix is totally fine. Environmental variables are however something that we need to handle with care in an attempt to bring consistency. The issue here is whether to let FuseSoC expand the variables or let the backend (e.g. Edalize do that). In most cases it doesn't really matter but when e.g. FuseSoC and the EDA tools reside on different machines, it's a bit different. A prime example of this would be when some EDA-tool-specific file needs to be linked in that resides in $EDA_TOOL_ROOT/some/internal/dir. We want to expand these kind of variables in edalize since the path to the EDA tool might be different from the FuseSoC machine. OTOH, setting something like $CACHE_SIZE is most likely intended to be expanded before generators are run. My thinking lately has been to introduce syntax for creating proper variables in core description files that would be expanded immediately and then let environment variables be expanded by the machine that runs the EDA tools (since they are variables that describe the environment after all). Problem with that is that environment variables in other places in the code base are already expanded by FuseSoC so maybe that ship has sailed already. Perhaps we need to still introduce a syntax for variables, but also add syntax for marking environment variables that are to be expanded later. And just let the current environment variable handling be as is.
I don't know really..
It's impossible atm to call a tool with an environment variable in the flags. This env var expansion is happening at core parsing time, right? So it doesn't cover the use case you're concerned about. If you want to deal with that later then you can just add that capability. This is the bare minimum functionality that's needed. If later you need to change it to pass the variables through EDAM to the tool then the current functionality isn't going to be broken unless a generator overwrites an env var. This patch seems completely reasonable to merge.
This wasn't intended to be closed. It happened when I renamed the default branch. Please reopen if it's still relevant