make: preserve tool *_EXE env vars across UNSET_AND_MAKE#4159
Open
oharboe wants to merge 2 commits intoThe-OpenROAD-Project:masterfrom
Open
make: preserve tool *_EXE env vars across UNSET_AND_MAKE#4159oharboe wants to merge 2 commits intoThe-OpenROAD-Project:masterfrom
oharboe wants to merge 2 commits intoThe-OpenROAD-Project:masterfrom
Conversation
Commit df79ba1 ("make: immediately expand deferred vars") changed the tool-path setup from export OPENROAD_EXE ?= $(abspath ...) to OPENROAD_EXE ?= $(abspath ...) export OPENROAD_EXE := $(OPENROAD_EXE) The := reassign gives the variable origin "file" even when the value was supplied through the environment. UNSET_VARIABLES_NAMES collects every file-origin variable and $(UNSET_AND_MAKE) unsets them in the bash wrapper before invoking the submake. That strips a caller- supplied OPENROAD_EXE / OPENSTA_EXE / YOSYS_EXE from the submake's environment, and variables.mk re-derives them via the $(FLOW_HOME)/../tools/install/... fallback — a path that does not exist in hermetic builds (e.g. bazel-orfs). Extend the unset exclusion list to cover OPENROAD%, OPENSTA%, YOSYS%, mirroring the KLAYOUT% entry already there for the same reason. This restores the pre-df79ba1d9 behavior for callers that pass tool paths through the environment, without reverting the immediate- expansion fix. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the variable exclusion list in flow/scripts/variables.mk to include OPENROAD%, OPENSTA%, and YOSYS%, which prevents these tool-related variables from being unset during sub-make calls. A review comment suggests also adding PYTHON% to this list to ensure that user-provided PYTHON_EXE overrides are preserved when the variable is re-assigned within the Makefile.
PYTHON_EXE follows the same `?=` then `export := $(PYTHON_EXE)` pattern as OPENROAD_EXE / OPENSTA_EXE / YOSYS_EXE, so a caller-supplied value is similarly stripped by UNSET_AND_MAKE because the re-export gives it origin "file". Add PYTHON% to the exclusion list alongside the other tool prefixes. Addresses gemini-code-assist review feedback on PR The-OpenROAD-Project#4159. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit df79ba1 ("make: immediately expand deferred vars") changed the tool-path setup from
to
The := reassign gives the variable origin "file" even when the value was supplied through the environment. UNSET_VARIABLES_NAMES collects every file-origin variable and$(UNSET_AND_MAKE) unsets them in the bash wrapper before invoking the submake. That strips a caller- supplied OPENROAD_EXE / OPENSTA_EXE / YOSYS_EXE from the submake's environment, and variables.mk re-derives them via the $ (FLOW_HOME)/../tools/install/... fallback — a path that does not exist in hermetic builds (e.g. bazel-orfs).
Extend the unset exclusion list to cover OPENROAD%, OPENSTA%, YOSYS%, mirroring the KLAYOUT% entry already there for the same reason.
This restores the pre-df79ba1d9 behavior for callers that pass tool paths through the environment, without reverting the immediate- expansion fix.