Skip to content

Expand environment variables in configuration files#888

Merged
elamdf merged 1 commit intoucb-bar:masterfrom
daniellovell:expand-envvars
Mar 6, 2025
Merged

Expand environment variables in configuration files#888
elamdf merged 1 commit intoucb-bar:masterfrom
daniellovell:expand-envvars

Conversation

@daniellovell
Copy link
Collaborator

It is common to define paths to collateral files with environment variables, but currently if you include paths in your config/spec .yml file that use environment vars, you will get an error.

Consider the following example, I would like to add a new library to vlsi.technology.extra_libraries:

Spec.yml

vlsi.technology.extra_libraries:
    ## Sensor ADC
    - library:
        gds_file: "$WORK_DIR/vlsi/sensor_adc_top.gds"
        lef_file: "$WORK_DIR/vlsi/sensor_adc_top_abstract.lef"
        verilog_synth: "$CHIP/vlsi/collaterals/utilities/scumvtop_gen/SENSOR_PLACE.v"
        nldm_liberty_file: "$WORK_DIR/vlsi/sensor_adc_top.lib"
        spice_file: "$CHIP/vlsi/collaterals/spice_netlists/SENSOR_PLACE.src.net"

If I attempt any operation using this library, I will get a ValueError because the envvars are not expanded

ValueError: Path $WORK_DIR/vlsi/sensor_adc_top.lib with prefix id \\
   $WORK_DIR did not match any tarballs or installs

This PR fixes the issue, by expanding all envvars in yaml & json files that are loaded by Hammer

Related PRs / Issues

Type of change:

  • Bug fix
  • New feature
  • Other enhancement

Impact:

  • Change to core Hammer
  • Change to a Hammer plugin
  • Other

Contributor Checklist:

  • Did you set master as the base branch?
  • Did you state the type-of-change/impact?
  • Did you delete any extraneous prints/debugging code?
  • (If applicable) Did you add documentation for the feature?
  • (If applicable) Did you update the poetry.lock file if you updated the requirements in pyproject.toml?
  • (If applicable) Did you add a unit test demonstrating the PR?
  • (If applicable) Did you run this through the e2e integration tests?
  • (If applicable) Did you update the submodules in e2e/ if this feature depends on updated plugins?

Copy link
Collaborator

@elamdf elamdf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@elamdf elamdf added this pull request to the merge queue Mar 6, 2025
Merged via the queue into ucb-bar:master with commit f157f1f Mar 6, 2025
4 checks passed
@sunjin-choi
Copy link

@daniellovell @elamdf I agree this is useful, but there could be some corner cases when subst meta action is applied to the variable?

@elamdf
Copy link
Collaborator

elamdf commented Mar 7, 2025

if the variable doesn't exist in the environment, expandvars will leave it untouched. So this would only be an issue if you had an environment variable like export technology.sky130.sky130A="/badpath", which I agree is possible but is pretty unlikely imo unless some serious hacking is going on (or if hammer is messing with environmental variables to keep some state)

@sunjin-choi
Copy link

I realized the concern is more so on the debuggability, esp with other meta actions.
As the unittest suite defines many of chained meta actions that I believe are used for the PDK setups, for example, a chain of lazysubst as described here could be difficult to back-trace the cause when one of them is overridden by environment variable (or if one is unaware of this update/behavior is unclear).

As a clarification question, environmental variable substitute would be at the higher priority than any other subst targets, right? @elamdf @daniellovell

@elamdf
Copy link
Collaborator

elamdf commented Mar 7, 2025

that's a good point- perhaps we should be doing a check to make sure that env variables aren't overriding lazy yaml variables (or at least printing a warning or something)

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

Successfully merging this pull request may close these issues.

4 participants