fusesoc icon indicating copy to clipboard operation
fusesoc copied to clipboard

Deprecate environment variable expansion in file names?

Open imphil opened this issue 4 years ago • 4 comments

Since d8ece6b5 file names in filesets can contain environment variables, which are expanded during parsing. I'd like to better understand the use case for this feature, and potentially get rid of it.

Why I think this feature is a bad idea:

  • It makes core files non-portable: depending on environment variables fusesoc will pick different files.
  • It encourages users to specify absolute paths (e.g. $HOME/something), which breaks package encapsulation (the ability to take the directory containing the core file and have all necessary information).

I wouldn't expect this feature to be widely used, since it is neither documented nor announced in the 1.8.2 release notes, where it appeared first (http://olofkindgren.blogspot.com/2018/06/fusesoc-182.html).

@olofk do you have an interesting use case for that, or can we deprecate it?

imphil avatar Jan 30 '21 21:01 imphil

The main reason was for getting files from EDA tool installations, e.g. many Xilinx libs require simulating with $XILINX_VIVADO/data/verilog/src/glbl.v. There are also other situations where you might need to refer to paths outside of FuseSoC's control and still want to avoid hardcoding an absolute path.

But it's still problematic for the reason you cite and I see a couple of open issues

  1. I would prefer to move the expansion to Edalize, but FuseSoC currently also needs to know if it's an absolute path, because it will not copy the file to the build dir in that case. It could even be argued that the expansion should be deferred to the actual tool invocation for tools that natively support env vars. This is important for supporting the future use cases of running FuseSoC on a different machine than Edalize or the EDA tool. Maybe we can have a policy where we never copy files with env vars in the path or something like that. Or add an is_absolute_path flag. Not sure. Need to look at use cases
  2. As you say, it's not very commonly used, although I believe there are cases where this is really needed. Related to this I want to somehow support variable substitution in the core description files. There are some very handy uses for this together with use flags. What I'm thinking is perhaps to replace the current syntax of $ENV_VAR with a mechanism for calling functions, like ${env(ENV_VAR)}. There have been a number of cases where it would have been handy to do some kind of transformation on strings in core description files, although I think the most pressing ones have been taken care of in other ways (like the copyto option for files which took away the need to know the relative path to the source directory by copying the file to a known relative location instead). Maybe it's time to put this on the roadmap. One thing that has always held it back is that I've been highly reluctant to invent a new inline DSL but haven't found a good existing fit. TCL and Lua are both too heavy for this. It could be that Google's CEL would be a good choice here but needs to be investigated further

So, no. I don't think we can deprecate it yet but I think we could at least move the expansion to Edalize and treat all paths that needs expansions as absolute paths which means we will not copy them to the build tree

olofk avatar Feb 11 '21 13:02 olofk

Also, let's push this past the 1.12 release

olofk avatar Feb 11 '21 13:02 olofk

For the Xilinx use case (referring to e.g. $XILINX_VIVADO/data/verilog/src/glbl.v) I would prefer if people just added a core file to e.g. $XILINX_VIVADO and then add this directory as FuseSoC libray.

But yeah, let's discuss this further after the 1.12 release.

imphil avatar Feb 12 '21 16:02 imphil

I don't think it's feasible to do that as tool installations can be decoupled from the design environment

olofk avatar Feb 25 '21 20:02 olofk