From 55f4cf832afe114d1624892e74cb1cb1d7d28a1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20Harboe?= Date: Fri, 16 Oct 2026 09:56:23 +0200 Subject: [PATCH 1/3] docs: Makefile contract, the rules a Makefile change is reviewed against MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit flow/Makefile keeps to a handful of conventions that were never written down: a do- target runs one step and builds nothing, dependencies live on the file targets, the graph does not depend on a variable, a step writes only files of its own, a design's variables are assigned with ?=, and so on. docs/contrib/MakefileContract.md states them as hard-stop rules, each with what breaks when it is broken, for automated review of AI changes to flow/Makefile, flow/scripts/*.mk, the stage scripts and variables.yaml. AGENTS.md (and CLAUDE.md, a link to it) points agents at it, and the Developer's Guide links it. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Øyvind Harboe --- AGENTS.md | 2 + docs/contrib/DeveloperGuide.md | 1 + docs/contrib/MakefileContract.md | 133 +++++++++++++++++++++++++++++++ docs/toc.yml | 2 + 4 files changed, 138 insertions(+) create mode 100644 docs/contrib/MakefileContract.md diff --git a/AGENTS.md b/AGENTS.md index f4cb490535..5f5145a24e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -3,3 +3,5 @@ See `README.md` for the general ORFS guide. The OpenROAD (OR) engine source resides in `tools/OpenROAD/` (git submodule). For C++/Tcl development, refer to `tools/OpenROAD/AGENTS.md`. + +A change to `flow/Makefile`, `flow/scripts/*.mk`, `flow/scripts/*.tcl` or `flow/scripts/variables.yaml` is checked against every rule in `docs/contrib/MakefileContract.md`. diff --git a/docs/contrib/DeveloperGuide.md b/docs/contrib/DeveloperGuide.md index 395c95ab3c..52ca85f395 100644 --- a/docs/contrib/DeveloperGuide.md +++ b/docs/contrib/DeveloperGuide.md @@ -3,6 +3,7 @@ - Integrate a new platform to OpenROAD Flow: [Guide](./PlatformBringUp.md). - Adding a new design: [Guide](../user/AddingNewDesign.md). - Continuous Integration: [Guide](./CI.md). +- Rules a change to the Makefile, the stage scripts or `variables.yaml` is reviewed against: [Makefile Contract](./MakefileContract.md). - How do I update the codebase? There are different ways to update your codebase depending on the method you installed it. We provide detailed instructions in this [guide](../user/FAQS.md). - How do I contribute? Follow our Git Quickstart guide [here](./GitGuide.md). diff --git a/docs/contrib/MakefileContract.md b/docs/contrib/MakefileContract.md new file mode 100644 index 0000000000..3f3c0e3d73 --- /dev/null +++ b/docs/contrib/MakefileContract.md @@ -0,0 +1,133 @@ +# Makefile Contract + +`flow/Makefile` is written in make, a rich programming language behind a +terse syntax: recursive and simple expansion, `=` against `:=` against `?=`, +the origin of a variable, exports to sub-makes, a graph built in one pass and +run in another. ORFS keeps the Makefile as simple as it can, and over time a +handful of conventions have grown up that keep it that way. Most of them were +never written down. + +This page writes them down as rules a review can check a change against: a +change to `flow/Makefile`, `flow/scripts/*.mk`, the stage scripts in +`flow/scripts/*.tcl` or `flow/scripts/variables.yaml`. It is written for +automated review of AI maintenance of these files: an agent that changes one +of them, or reviews such a change, checks it against every rule here. + +The rules are hard stops: a change that breaks one is wrong, whatever else it +fixes. Each rule says what breaks when it is broken. Existing code that breaks +a rule is debt to fix, not a precedent to follow. + +Some flows run the Makefile one step at a time, each step in a fresh tree that +holds only that step's inputs, and track the dependencies themselves; +[bazel-orfs](https://github.com/The-OpenROAD-Project/bazel-orfs) is one. +Several of the rules exist for them. + +## Targets + +- **A `do-` target never builds anything.** It runs its one step on the files + already in the tree. Its prerequisites are checks (`check-kepler-formal`), + the step's source files and one-time markers (`$(OBJECTS_DIR)/copyright.txt`): + never an order-only prerequisite, never a file another target writes. + *Breaks:* a flow that runs each `do-` target alone in a fresh tree rebuilds + the prerequisite every time. +- **Dependencies live only on the file targets and stage aliases** + (`$(RESULTS_DIR)/2_1_floorplan.odb`, `synth`, `floorplan`, ...), never on a + `do-` target. The file target checks the dependencies and calls the `do-` + target to run the step. + *Breaks:* a flow that tracks dependencies itself cannot run one step on + existing results. +- **`open_*`, `gui_*`, `web_*` and `run` never build anything.** `run` writes + `$(LOG_DIR)/$(RUN_LOG_NAME_STEM).log` and nothing under `RESULTS_DIR`, + `REPORTS_DIR` or `OBJECTS_DIR`. + *Breaks:* opening or scripting a stage changes the tree it looks at, and a + caller that runs several `make run` at once on one tree gets them writing + the same files. +- **The graph never depends on a variable.** No `ifeq` chooses which steps run + or which files they write. A step with nothing to do for a design runs + anyway and copies its `.odb` and `.sdc` through; the choice is made in its + `.tcl`. + *Breaks:* a flow that names a step's outputs before running it gets a + different set for each design. + +## Steps + +- **A step never writes next to its inputs or into one.** It writes new files + under names of its own, as `2_floorplan.odb` → `3_place.odb`. + *Breaks:* a rerun reads what the last run wrote, and a flow that keeps + inputs read-only cannot run the step. +- **A step never skips an output because the file exists.** "Up to date" is + the file target's decision. + *Breaks:* a tree that kept a file from an earlier run and a fresh tree give + different results. +- **A step never reads a file it does not declare.** + *Breaks:* it works under `make synth`, where an earlier target happened to + build the file, and fails when the step runs alone. +- **A step never reads an input it does not use.** + *Breaks:* every change to that input reruns the step and every step after + it. +- **A step never leaves configuration for a later step.** A command gets its + options in its own `log_cmd` line; a setting it needs is set right before + the call and reset right after. State that crosses steps is in the `.odb` or + another result file. + *Breaks:* the later step's log does not show what it ran with, and a + checkpoint changes when the design did not. +- **A step never draws an unseeded random number.** The seed comes from a + variable and is recorded. + *Breaks:* caching a step's result, the QoR check against a baseline, and + bisecting a regression. +- **A failing step never calls `exit`.** It writes `-failed.odb` and + calls `error`, as `flow/scripts/global_place.tcl` does. + *Breaks:* the log and the exit status stop saying where the step failed, + and there is no checkpoint to inspect. +- **A `PRE__TCL` hook never runs before the design is loaded.** Hooks + run through `source_step_tcl` in `flow/scripts/util.tcl`, after + `load_design`. + *Breaks:* the hook has no design to act on. + +## Variables + +- **A variable a design sets is never assigned with `=` in the Makefile.** It + is assigned with `?=`. `RUN_CMD` in `flow/scripts/variables.mk` is the + exception; it can be set only on the make command line. + *Breaks:* a flow that passes a design's variables in `DESIGN_CONFIG` as + `export VAR?=value` lines is overridden. +- **A variable the environment supplies is never exported with + `export VAR := $(VAR)`.** It is exported bare, `export VAR`, and its default + is a `?=` fallback to the in-tree install. + *Breaks:* `:=` rebinds the variable to origin "file", `UNSET_AND_MAKE` + unsets it before the sub-make, and the fallback fires there with an in-tree + path that a flow supplying its own tools does not have (the "HERE BE + DRAGONS" comment in `flow/scripts/variables.mk`). +- **Only the Makefile sets `RESULTS_DIR`, `LOG_DIR`, `REPORTS_DIR` and + `OBJECTS_DIR`.** It derives them from `WORK_HOME`, which a flow sets. A + variable a flow adds never reuses one of these names. + *Breaks:* a flow that relocates the tree through `WORK_HOME` finds results + where the Makefile did not put them. +- **A variable is never missing from `flow/scripts/variables.yaml`.** Its entry + has its default, when it has one, and `stages:` when not every stage reads + it. + *Breaks:* a flow that checks variable names against `variables.yaml` + refuses it, and a flow that passes each stage only its own variables + passes it nowhere, or everywhere. +- **A variable that has a default is never read through an existence check.** + It is read as `$::env(VAR)`, never through `env_var_exists_and_non_empty` + with a fallback. A boolean is `0`/`1` and has a default. + *Breaks:* a misspelled or unpropagated variable takes the other branch + quietly, instead of stopping at the line that reads it. +- **A script never overwrites a variable that is set.** It sets one only when + it is unset. + *Breaks:* the design's or the user's value is ignored. +- **A platform default is never repeated in a design's `config.mk`.** It is in + the platform's `config.mk`, assigned with `?=`. + *Breaks:* a change to the platform default misses the designs that copied + it, and a design can no longer override it. +- **A variable never has a negative name** (`LEC_CHECK=1`, never + `DISABLE_LEC_CHECK`), **never duplicates an existing one** (`ADDITIONAL_LIBS` + for an extra library), **and is never a default-off feature toggle** that no + CI design sets. A variable with no effect is deleted, not deprecated. + *Breaks:* double negatives in settings, two variables for one choice, and + code no CI run exercises. +- **A variable's value is never computed with `$(shell awk ...)` or + `$(shell sed ...)`.** That logic is in Python with unit tests. + *Breaks:* a heuristic nobody can test, read, or run on a machine without + that tool. diff --git a/docs/toc.yml b/docs/toc.yml index 92b208b6b0..ca7f32a6cc 100644 --- a/docs/toc.yml +++ b/docs/toc.yml @@ -45,6 +45,8 @@ entries: title: Guide to Integrate a New Platform into the OpenROAD Flow - file: contrib/CI.md title: CI Guide + - file: contrib/MakefileContract.md + title: Makefile Contract - file: contrib/EndpointSlackTracking.md title: Tracking Endpoint Slack Across Flow Steps From ee2ed99822304223502337c16e9fad3fb98777a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20Harboe?= Date: Fri, 16 Oct 2026 21:13:22 +0200 Subject: [PATCH 2/3] docs: Makefile contract, address review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - State that any place where the code disagrees with a rule is technical debt, from an expedient solution or older code, instead of listing cases that would go out of date. - Limit the variables.yaml rule to variables a design, platform or user sets, or a stage script reads; Makefile-internal helpers are not listed. - Add flow/platforms/*/config.mk and flow/designs/**/config.mk to the scope in MakefileContract.md and AGENTS.md. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Øyvind Harboe --- AGENTS.md | 2 +- docs/contrib/MakefileContract.md | 18 ++++++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5f5145a24e..e9cd132d84 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,4 +4,4 @@ See `README.md` for the general ORFS guide. The OpenROAD (OR) engine source resides in `tools/OpenROAD/` (git submodule). For C++/Tcl development, refer to `tools/OpenROAD/AGENTS.md`. -A change to `flow/Makefile`, `flow/scripts/*.mk`, `flow/scripts/*.tcl` or `flow/scripts/variables.yaml` is checked against every rule in `docs/contrib/MakefileContract.md`. +A change to `flow/Makefile`, `flow/scripts/*.mk`, `flow/scripts/*.tcl`, `flow/scripts/variables.yaml`, `flow/platforms/*/config.mk` or `flow/designs/**/config.mk` is checked against every rule in `docs/contrib/MakefileContract.md`. diff --git a/docs/contrib/MakefileContract.md b/docs/contrib/MakefileContract.md index 3f3c0e3d73..524025a7b5 100644 --- a/docs/contrib/MakefileContract.md +++ b/docs/contrib/MakefileContract.md @@ -9,13 +9,17 @@ never written down. This page writes them down as rules a review can check a change against: a change to `flow/Makefile`, `flow/scripts/*.mk`, the stage scripts in -`flow/scripts/*.tcl` or `flow/scripts/variables.yaml`. It is written for +`flow/scripts/*.tcl`, `flow/scripts/variables.yaml`, or the `config.mk` of a +platform (`flow/platforms/*/config.mk`) or a design +(`flow/designs/**/config.mk`). It is written for automated review of AI maintenance of these files: an agent that changes one of them, or reviews such a change, checks it against every rule here. The rules are hard stops: a change that breaks one is wrong, whatever else it -fixes. Each rule says what breaks when it is broken. Existing code that breaks -a rule is debt to fix, not a precedent to follow. +fixes. Each rule says what breaks when it is broken. Any place where the +existing code disagrees with a rule is technical debt, whether an expedient +solution or older code written before the rule: debt to fix, not a precedent +to follow. Some flows run the Makefile one step at a time, each step in a fresh tree that holds only that step's inputs, and track the dependencies themselves; @@ -103,9 +107,11 @@ Several of the rules exist for them. variable a flow adds never reuses one of these names. *Breaks:* a flow that relocates the tree through `WORK_HOME` finds results where the Makefile did not put them. -- **A variable is never missing from `flow/scripts/variables.yaml`.** Its entry - has its default, when it has one, and `stages:` when not every stage reads - it. +- **A variable a design, platform or user sets, or a stage script reads, is + never missing from `flow/scripts/variables.yaml`.** Its entry has its + default, when it has one, and `stages:` when not every stage reads it. + Helpers internal to the Makefile, such as `DO_STEP_DEPS_*` and + `UNSET_AND_MAKE`, are not listed. *Breaks:* a flow that checks variable names against `variables.yaml` refuses it, and a flow that passes each stage only its own variables passes it nowhere, or everywhere. From 64625134f790d91bd8c729d00d485c08c7cda04a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20Harboe?= Date: Fri, 16 Oct 2026 21:14:45 +0200 Subject: [PATCH 3/3] docs: Makefile contract, Makefile-only variables need not be in variables.yaml MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WORK_HOME and other variables only the Makefile reads may be documented elsewhere; variables.yaml covers what a design or platform sets or a stage script reads. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Øyvind Harboe --- docs/contrib/MakefileContract.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/docs/contrib/MakefileContract.md b/docs/contrib/MakefileContract.md index 524025a7b5..ae604ca719 100644 --- a/docs/contrib/MakefileContract.md +++ b/docs/contrib/MakefileContract.md @@ -107,11 +107,12 @@ Several of the rules exist for them. variable a flow adds never reuses one of these names. *Breaks:* a flow that relocates the tree through `WORK_HOME` finds results where the Makefile did not put them. -- **A variable a design, platform or user sets, or a stage script reads, is - never missing from `flow/scripts/variables.yaml`.** Its entry has its - default, when it has one, and `stages:` when not every stage reads it. - Helpers internal to the Makefile, such as `DO_STEP_DEPS_*` and - `UNSET_AND_MAKE`, are not listed. +- **A variable a design or platform sets, or a stage script reads, is never + missing from `flow/scripts/variables.yaml`.** Its entry has its default, + when it has one, and `stages:` when not every stage reads it. A variable + only the Makefile reads, such as `WORK_HOME`, or that is internal to it, + such as `DO_STEP_DEPS_*` and `UNSET_AND_MAKE`, may be documented elsewhere + and need not be listed. *Breaks:* a flow that checks variable names against `variables.yaml` refuses it, and a flow that passes each stage only its own variables passes it nowhere, or everywhere.