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

Deprecate environment variable expansion in file names? #487

Open
imphil opened this issue Jan 30, 2021 · 4 comments
Open

Deprecate environment variable expansion in file names? #487

imphil opened this issue Jan 30, 2021 · 4 comments

Comments

@imphil
Copy link
Collaborator

imphil commented Jan 30, 2021

Since d8ece6b 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 imphil added category:capi2 CAPI2-related issues status:needs-info feedback from someone is required. The issue/PR text gives more details. labels Jan 30, 2021
@imphil imphil added this to the 1.12.0 milestone Jan 30, 2021
@olofk
Copy link
Owner

olofk commented Feb 11, 2021

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

olofk commented Feb 11, 2021

Also, let's push this past the 1.12 release

@imphil imphil removed category:capi2 CAPI2-related issues status:needs-info feedback from someone is required. The issue/PR text gives more details. labels Feb 12, 2021
@imphil
Copy link
Collaborator Author

imphil commented Feb 12, 2021

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 imphil removed this from the 1.12.0 milestone Feb 12, 2021
@olofk
Copy link
Owner

olofk commented Feb 25, 2021

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

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

2 participants