Conversation
Previous approach apparently caused some externally-defined variables to become overwritten, and modifications made in 72eca88 effectively reverted back to the old behaviour of spawning 28k shells for each `make` invocation. Try to cut that number down to 41 again by explicitly checking if the variables have been externally defined. Signed-off-by: ctkiku <kim.kuparinen@tuni.fi>
![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 flow/scripts/variables.mk to replace the conditional variable assignment operator ?= with explicit ifeq ($(origin VAR), undefined) checks for PYTHON_EXE, OPENROAD_EXE, OPENSTA_EXE, and YOSYS_EXE. This ensures that these variables are only assigned default values if they are completely undefined, preventing unintended overrides. There are no review comments to assess, and I have no additional feedback to provide.
|
Let me test it with bazel-orfs, which is my use-case |
🔍 QoR checkMetrics reflect the PR merge build — i.e. what will land on the target branch. Commit 62 design(s) checked — 9 with regression(s), 0 without a comparable baseline. ❌ asap7/aes-mbff base — 2 failing metric(s)
❌ asap7/ibex base — 4 failing metric(s)
❌ asap7/mock-cpu base — 6 failing metric(s)
❌ gt2n/gcd base — 7 failing metric(s)
❌ gt2n/jpeg base — 4 failing metric(s)
❌ ihp-sg13g2/gcd base — 10 failing metric(s)
❌ ihp-sg13g2/jpeg base — 5 failing metric(s)
❌ nangate45/cva6 base — 5 failing metric(s)
❌ sky130hd/ibex base — 6 failing metric(s)
|
|
Thanks. I tested this: it works, and it doesn't break bazel-orfs. It made me think about a direction that could make the Makefile more environment-agnostic, so that bazel-orfs, Nix and whatever comes next are all first-class, with no special case for any of them. For one
ORFS already has the mechanism, and bazel-orfs uses it: the environment sets devShells.default = pkgs.mkShell {
buildInputs = [ ... ];
OPENROAD_EXE = "${openroad.packages.${system}.default}/bin/openroad";
OPENSTA_EXE = "${openroad.packages.${system}.default}/bin/sta";
YOSYS_EXE = "${yosys.packages.${system}.default}/bin/yosys";
PYTHON_EXE = "${pkgs.python3}/bin/python3";
};
The comment block can carry that as the rule for the future: the environment supplies tool paths, whether it's bazel-orfs, a Nix shell or a user; defaults here never call One behaviour changes: a Nix shell other than ORFS's flake, with openroad on Would you be up for reshaping the PR that way? I don't have Nix to test the flake half. Two more of the same kind, if you're in there anyway:
|
|
@hzeller @maliberty FYI. TL;DR I think we should generalize and narrow the interface of ORFS to support nix and bazel-orfs as first class citizens rather than add more conditional code to the makefiles. |
3 participants
Previous approach apparently caused some externally-defined variables to become overwritten, and modifications made in 72eca88 effectively reverted back to the old behaviour of spawning 28k shells for each
makeinvocation. Try to cut that number down to 41 again by explicitly checking if the variables have been externally defined.@oharboe Does this look alright?
ifeq ($(origin ...should be identical to?=according to the GNU Make manual, and passing variables from outside likePYTHON_EXE=... make ...seems to work on my machine.