Skip to content

fix make issue missing variables#4230

Merged
maliberty merged 1 commit into
The-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:secure-fix-variables
May 15, 2026
Merged

fix make issue missing variables#4230
maliberty merged 1 commit into
The-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:secure-fix-variables

Conversation

@gudeh
Copy link
Copy Markdown
Contributor

@gudeh gudeh commented May 14, 2026

Lately I have been stumbling on two different issues when executing artifacts generated with make issue. This PR fixes both.

They are:

  • [ERROR GUI-0070] can't read "::env(OPENROAD_HIERARCHICAL)": no such variable
  • [ERROR GUI-0070] can't read "::env(KEPLER_FORMAL_EXE)": no such variable

OPENROAD_HIERARCHICAL is excluded from the check with OPENROAD%, so it was converted into explicit exclusions.

KEPLER_FORMAL_EXE is correctly stripped from the tarball, but LEC_CHECK=1 is exported, run-me tries to call kepler-formal without knowing where it is. Added a lec_check_enabled helper that gates on both.

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
@gudeh gudeh self-assigned this May 14, 2026
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a lec_check_enabled helper to centralize Logic Equivalence Checking (LEC) logic and updates the variable exclusion list in the build system. Feedback suggests adding a warning when LEC is requested but the executable is missing to improve observability, and refactoring the growing list of excluded variables in the Makefile for better maintainability.

Comment on lines +1 to +7
proc lec_check_enabled { } {
return [expr {
[env_var_equals LEC_CHECK 1]
&& [info exists ::env(KEPLER_FORMAL_EXE)]
&& [file executable $::env(KEPLER_FORMAL_EXE)]
}]
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

While this helper correctly prevents a crash when KEPLER_FORMAL_EXE is missing (which is expected in make issue artifacts), it will silently skip LEC even if the user explicitly requested it via LEC_CHECK=1 in a normal environment. Adding a warning message when the executable is missing but the check is enabled would improve observability.

proc lec_check_enabled { } {
  if { ![env_var_equals LEC_CHECK 1] } {
    return 0
  }
  if { ![info exists ::env(KEPLER_FORMAL_EXE)] || ![file executable $::env(KEPLER_FORMAL_EXE)] } {
    puts "\[WARNING\] LEC_CHECK is enabled but KEPLER_FORMAL_EXE is not found or not executable. Skipping LEC."
    return 0
  }
  return 1
}

Comment thread flow/scripts/variables.mk

define get_variables
$(foreach V, $(.VARIABLES),$(if $(filter-out $(1), $(origin $V)), $(if $(filter-out .% %QT_QPA_PLATFORM% KLAYOUT% OPENROAD% OPENSTA% PYTHON% YOSYS% GENERATE_ABSTRACT_RULE% do-step% do-copy% OPEN_GUI% OPEN_GUI_SHORTCUT% SUB_MAKE% UNSET_VARS% export%, $(V)), $V$ )))
$(foreach V, $(.VARIABLES),$(if $(filter-out $(1), $(origin $V)), $(if $(filter-out .% %QT_QPA_PLATFORM% KLAYOUT% OPENROAD_EXE OPENROAD_ARGS OPENROAD_CMD OPENROAD_NO_EXIT_CMD OPENROAD_GUI_CMD OPENROAD_WEB_CMD OPENROAD_IS_VALID OPENSTA% PYTHON% YOSYS% GENERATE_ABSTRACT_RULE% do-step% do-copy% OPEN_GUI% OPEN_GUI_SHORTCUT% SUB_MAKE% UNSET_VARS% export%, $(V)), $V$ )))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The list of excluded variables is becoming quite long and difficult to maintain. Consider refactoring this into a separate variable to improve readability and make it easier to add or remove patterns in the future.

@gudeh gudeh requested a review from maliberty May 14, 2026 23:31
@maliberty maliberty merged commit 69ae727 into The-OpenROAD-Project:master May 15, 2026
8 checks passed
@maliberty maliberty deleted the secure-fix-variables branch May 15, 2026 14:55
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.

2 participants