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

Environment variable behaviour changed between fusesoc 2.1 and 2.2 #641

Open
GCHQDeveloper211 opened this issue Aug 10, 2023 · 7 comments
Open

Comments

@GCHQDeveloper211
Copy link

The previous behaviour, was that for an environment variable in a file name (i.e looking for the glbl.v file for xilinx) this would be expanded all the time. As of the update, the environment variables are no longer expanded, unless there is a --resolve-env-vars-early argument passed in.

Is this expected behaviour, and if it is, is there a way to set this in the core file, as opposed to part of the CLI args.

@GCHQDeveloper211 GCHQDeveloper211 changed the title Environemnt variable behaviour changed between fusesoc 2.1 and 2.2 Environment variable behaviour changed between fusesoc 2.1 and 2.2 Aug 15, 2023
@ttobsen
Copy link

ttobsen commented Aug 24, 2023

Same problem here. Unfortunately, this is a breaking change for our complex CI system. 😞

@olofk
Copy link
Owner

olofk commented Aug 28, 2023

This is an old issue caused by different people wanting to use env var for different things. Both camps have been arguing for their use case and traditionally FuseSoC has been pretty incosistent which is a pretty bad compromise. The two main use cases are

  1. Resolving early to replace a hardcoded value in a core description file
  2. Resolving late (i.e. in Edalize) to let the environment where the EDA tool runs fill in the env var

The problem is that there hasn't been a way to distinguish between these use cases. We introduced --resolve-env-vars-early as an attempt to globally set 1 or 2, but my long-term plan has always been to introduce variables in the .core files (something like {{ varname }}) to support use case 1 and default to not expanding env vars anywhere. But I could easily be persuaded to do the opposite and require users to explicitly mark env vars that shouldn't be expanded by FuseSoC if that's what the users prefer. I do think the first option is cleaner though.

I apologize for adding a breaking change here. I didn't realize this. And I'm afraid there might be more places this might happen since env var handling has been too inconsistent traditionally. The important thing that I see at this point is to outline a proper solution and work towards that. Help appreciated :)

@ttobsen
Copy link

ttobsen commented Aug 28, 2023

Thanks for clearification! 👍

@GCHQDeveloper137
Copy link
Contributor

GCHQDeveloper137 commented Aug 29, 2023

Thanks for the information, Olof, that makes a lot of sense. For the use case that was breaking for our team specifically, it was case 1, we wanted to use different vendor tools versions and switch between some of their supplied sources, for example. We can fairly easily update our CI system to pass in --resolve-env-vars-early to get that working, and devs can do so manually (with a possible short failure first for anyone with a poor memory like myself!). It sounds like having core file variables might be a way around this, if they can interact with the environment somehow.

A thought has just occurred, would it make sense to look at reading options from configuration files? If you wanted to get fancy, you could have a system one (say in /etc) and then user-specific ones (in home directories), to set options by default? I know this might not be sensible for this one option if the longer-term plan is to move to a different way of handling variables, but I wonder if there's any appetite for this in the general case for other options?

@olofk
Copy link
Owner

olofk commented Aug 29, 2023

Yes, the intention is to have vars that can be settable on the command line (either like flags, or an actual expansion on the flags concept). And if you can set them on the command line, then the shell will take care of the env vars expansion. Something like fusesoc ... --var=$VALUE. There are probably different ways to do this, so I think it's good to work out this concept so all requirements are being addressed properly.

Regarding reading options from config files, that's also something I think makes a lot of sense. Some people always want to run with --verbose and some always wants --no-export. There has been suggestions to allow setting these in the core files, but I think that's the wrong place to have usage-specific options. We already have fusesoc.conf (which btw already is fancy and loads from /etc/fusesoc.conf, ~/.config/fusesoc/fusesoc.conf and ./fusesoc.conf ). I think this could be a prettty low-effort addition which has many good uses.

@GCHQDeveloper137
Copy link
Contributor

Interesting you mentioned that, as it gave me an idea for distributing common libraries across multiple development hosts using /etc/fusesoc.conf. I found (just through empirical testing) that at present, it looks like it only looks at /etc/fusesoc.conf if there is no ~/.config/fusesoc/fusesoc.conf, but it always looks at ./fusesoc.conf and merges that into whichever of the other two is present. I just thought I'd mention that in case anyone sees this and starts to work on this!

@olofk
Copy link
Owner

olofk commented Sep 13, 2023

@GCHQDeveloper137 Good catch. There could very well be some inconsistencies there as well.Should probably be fixed and either way properly documented

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

4 participants