From 676971866fd07699ceb1be586b74f34fc6dc2433 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20Harboe?= Date: Fri, 16 Oct 2026 21:36:46 +0200 Subject: [PATCH 1/3] docs: Tcl contract, the rules a stage script change is written and reviewed against MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Write down the idioms flow/scripts/*.tcl keeps to as hard-stop rules, each with what breaks when it is broken, plus the review suggestions on these scripts that are declined again and again, so that a contributor's AI writes code consistent with the existing scripts. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Øyvind Harboe --- AGENTS.md | 2 + docs/contrib/DeveloperGuide.md | 1 + docs/contrib/TclContract.md | 119 +++++++++++++++++++++++++++++++++ docs/toc.yml | 2 + 4 files changed, 124 insertions(+) create mode 100644 docs/contrib/TclContract.md diff --git a/AGENTS.md b/AGENTS.md index f4cb490535..40d7b4da90 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,4 +2,6 @@ See `README.md` for the general ORFS guide. +A change to `flow/scripts/*.tcl` is written and reviewed against every rule in `docs/contrib/TclContract.md`. + The OpenROAD (OR) engine source resides in `tools/OpenROAD/` (git submodule). For C++/Tcl development, refer to `tools/OpenROAD/AGENTS.md`. diff --git a/docs/contrib/DeveloperGuide.md b/docs/contrib/DeveloperGuide.md index 395c95ab3c..5d71f747ee 100644 --- a/docs/contrib/DeveloperGuide.md +++ b/docs/contrib/DeveloperGuide.md @@ -5,6 +5,7 @@ - Continuous Integration: [Guide](./CI.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). +- Rules a change to the stage scripts in `flow/scripts/*.tcl` is written and reviewed against: [Tcl Contract](./TclContract.md). ## Timing and Logging (`run_command.py`) diff --git a/docs/contrib/TclContract.md b/docs/contrib/TclContract.md new file mode 100644 index 0000000000..a99e9442c0 --- /dev/null +++ b/docs/contrib/TclContract.md @@ -0,0 +1,119 @@ +# Tcl Contract + +The stage scripts in `flow/scripts/*.tcl` are short, and they are read more +than they are written: a user who wants to know what a stage does, or which +knob to turn, reads its script. ORFS keeps them consistent with each other so +that knowing one script is most of the way to knowing the next. Over time a +set of idioms has grown up for doing that, and most of them were never +written down. + +This page writes them down as rules a change to `flow/scripts/*.tcl` is +written and reviewed against. It is written for AI authoring and automated +review: an agent that changes a stage script, or reviews such a change, +checks it against every rule here, and against the rules for steps and +variables in the [Makefile Contract](./MakefileContract.md), which apply to +the stage scripts too and are not repeated here. A contributor whose agent +follows both pages writes code that reads like the code around it, and gets +through review without a maintainer having to restate them. + +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. 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. + +## Idiom + +- **A script never writes its own version of an idiom `util.tcl` already + has.** An optional flag is added with `append_env_var`, a command that does + real work runs through `log_cmd`, a result is written with `orfs_write_db` + and `orfs_write_sdc`. An idiom a change needs in two places becomes a proc + in `util.tcl`. + *Breaks:* each copy grows its own small bug, and nobody can grep for the + places that do the same thing. +- **A command's arguments are never hidden from the log.** They are built in + a list and run through `log_cmd`, so the log shows the full command line. + A `_ARGS` variable replaces the arguments the flow would pass, so a + user can copy the line from the log into `config.mk` and edit it. + *Breaks:* a user cannot see what a step ran with, or reproduce it. +- **A proc never takes its input from a global or the caller's scope.** It + takes it as an argument and returns its result. State that must last + across steps is in the `.odb`. + *Breaks:* a reader cannot tell what a proc depends on, and the result + changes with the order scripts are `source`d in. +- **A script never repeats an OpenROAD default.** It passes an option only + when ORFS wants a value other than the tool's. + *Breaks:* a change to the default in OpenROAD has no effect in ORFS, and + the two drift apart. +- **A script never trades an explicit variable for a clever one.** A flag + that means one thing (`has_clock`) stays a flag; an empty string or a + sentinel value never stands in for it. + *Breaks:* the reader has to work out what the special value means. +- **Tcl is never written with tabs, quotes or braces it does not need, or + `[...]` around an `expr` condition.** `lappend` takes several values at + once. `tclint` and `tclfmt` (`tclint.toml`) check the rest in CI. + *Breaks:* the CI lint, and a reader who stops to work out why the extra + syntax is there. + +## Failure + +- **A script never guards a read the flow guarantees.** A variable with a + default in `flow/scripts/variables.yaml` is always set, and an input file + the step declares is always there: no `file exists` before reading it, no + `env_var_exists_and_non_empty` or `env_var_or_empty` fallback for it. The + read fails, loudly, at the line that does it. + *Breaks:* the guard is a code path no CI run exercises, and a missing + variable or file shows up later, somewhere else, as a wrong result. +- **A failing step never writes the file a successful step writes.** It + writes `-failed.odb` and calls `error`, as + `flow/scripts/global_place.tcl` does. + *Breaks:* make, and any flow built on it, takes the file as proof that the + step succeeded. +- **A script never adds error numbers or checks that belong in OpenROAD.** + OpenROAD allocates error numbers; ORFS reports with `error`. A check on + what a tool did belongs in the tool. + *Breaks:* numbers that collide with OpenROAD's, and a check that only one + flow runs. + +## Interface + +- **A result file's name and format never change as a side effect.** The + names under `RESULTS_DIR` and the format of what is written there are part + of ORFS's interface to users and other flows; a change to one is its own + pull request, called out as breaking. + *Breaks:* every flow and script that reads the old name. +- **`open.tcl` never writes a file.** Opening a stage, in the GUI or on the + console, only reads. A variable such as `GUI_TIMING` means the same thing + in both. + *Breaks:* looking at a result changes the tree it is in. +- **A refactor or cleanup never changes behavior.** A change in what a step + does is its own pull request, with its own QoR check. + *Breaks:* a regression hides inside a change described as a no-op, and the + review looks for it in the wrong place. + +## Review suggestions to decline + +Automated reviewers make some suggestions on these scripts again and again +that break the rules above. An agent writing a change does not take them, and +an agent reviewing one does not make them. + +- **"Crashes if the variable is undefined; use `env_var_exists_and_non_empty`, + `env_var_or_empty` or `env_var_equals`."** Not when the variable has a + default in `variables.yaml`: it is always set, and a crash is what the + flow wants when it is not. +- **"Check that the file exists before reading it."** The read already fails, + with the tool's own message. +- **"Keep the stack trace with `error $msg $::errorInfo`."** OpenSTA's command + layer loses the Tcl stack trace anyway. +- **"Write the file under `RESULTS_DIR`."** Read the callee first: + `write_lec_verilog` in `flow/scripts/formal_check.tcl` adds `RESULTS_DIR` + itself, and the suggestion doubles it. +- **"Make this robust for a case the flow does not have,"** such as a second + `read_timing` in one GUI session, or a variable for one design's special + case. Each adds code no CI run exercises, or changes behavior. +- **A rewrite of a yosys `select` expression.** Run it before taking it; + reviewers and tests have disagreed with such suggestions. + +Suggestions worth taking are real defects: a loop that is quadratic on large +nets, parsing that breaks on input the flow produces, a command that does +real work outside `log_cmd`. diff --git a/docs/toc.yml b/docs/toc.yml index 92b208b6b0..7a58dcc91b 100644 --- a/docs/toc.yml +++ b/docs/toc.yml @@ -41,6 +41,8 @@ entries: entries: - file: contrib/GitGuide.md title: Git Quickstart + - file: contrib/TclContract.md + title: Tcl Contract - file: contrib/PlatformBringUp.md title: Guide to Integrate a New Platform into the OpenROAD Flow - file: contrib/CI.md From 1072ac72ebfdf3c477ce8afb9c13c1f04a1405e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20Harboe?= Date: Fri, 16 Oct 2026 21:42:55 +0200 Subject: [PATCH 2/3] docs: TclContract: quote the yosys select item like the others MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 Signed-off-by: Øyvind Harboe --- docs/contrib/TclContract.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contrib/TclContract.md b/docs/contrib/TclContract.md index a99e9442c0..41c71b9d48 100644 --- a/docs/contrib/TclContract.md +++ b/docs/contrib/TclContract.md @@ -111,7 +111,7 @@ an agent reviewing one does not make them. - **"Make this robust for a case the flow does not have,"** such as a second `read_timing` in one GUI session, or a variable for one design's special case. Each adds code no CI run exercises, or changes behavior. -- **A rewrite of a yosys `select` expression.** Run it before taking it; +- **"Rewrite this yosys `select` expression."** Run it before taking it; reviewers and tests have disagreed with such suggestions. Suggestions worth taking are real defects: a loop that is quadratic on large From bfbfde85299310250b5cdb262772461d6bb77908 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20Harboe?= Date: Wed, 21 Oct 2026 06:46:42 +0200 Subject: [PATCH 3/3] docs: TclContract: which ARGS replace and which append; why no error numbers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _ARGS was described as replacing the flow's arguments, which only CTS_ARGS, RTLMP_ARGS and DETAILED_ROUTE_ARGS do; the others are appended. The rule now names both, and that a change never switches one into the other. The error-number rule keeps its point and says why: message IDs are OpenROAD's, unique per tool and checked there; nothing checks ORFS's, whose errors come from many tools. FLW 25 already names two messages, and GPL 200 sits in GPL's own range. The numbered messages already in the scripts stay until a change of their own removes them. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Øyvind Harboe --- docs/contrib/TclContract.md | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/docs/contrib/TclContract.md b/docs/contrib/TclContract.md index 41c71b9d48..9ec7ab1623 100644 --- a/docs/contrib/TclContract.md +++ b/docs/contrib/TclContract.md @@ -33,9 +33,14 @@ to follow. places that do the same thing. - **A command's arguments are never hidden from the log.** They are built in a list and run through `log_cmd`, so the log shows the full command line. - A `_ARGS` variable replaces the arguments the flow would pass, so a - user can copy the line from the log into `config.mk` and edit it. - *Breaks:* a user cannot see what a step ran with, or reproduce it. + A `_ARGS` variable either replaces the arguments the flow would pass + (`CTS_ARGS`, `RTLMP_ARGS`, `DETAILED_ROUTE_ARGS`) or is appended to them + (`GLOBAL_PLACEMENT_ARGS`, `PLACE_PINS_ARGS`, `BUFFER_PORTS_ARGS`, + `CLUSTER_FLOPS_ARGS`, `DETAIL_PLACEMENT_ARGS`); either way a user can copy + the line from the log into `config.mk` and edit it. Which of the two a + step does is part of its interface, and a change never switches it. + *Breaks:* a user cannot see what a step ran with, or reproduce it; a + `config.mk` written for one behavior silently does something else. - **A proc never takes its input from a global or the caller's scope.** It takes it as an argument and returns its result. State that must last across steps is in the `.odb`. @@ -70,10 +75,17 @@ to follow. *Breaks:* make, and any flow built on it, takes the file as proof that the step succeeded. - **A script never adds error numbers or checks that belong in OpenROAD.** - OpenROAD allocates error numbers; ORFS reports with `error`. A check on - what a tool did belongs in the tool. - *Breaks:* numbers that collide with OpenROAD's, and a check that only one - flow runs. + Message IDs are OpenROAD's: each tool owns its range, and OpenROAD checks + that every ID is unique (`docs/contrib/Logger.md`). Nothing checks ORFS's, + and its errors come from many tools besides OpenROAD (yosys, the memory + generators, Python, Tcl, make), so ORFS reports with `error` and a message + that says what failed. The numbered messages already in the scripts + (`FLW`, and `GPL 200` in `global_place.tcl`) are not a precedent; they + stay until a change of their own removes them, and `genMetrics.py` reads + one. A check on what a tool did belongs in the tool. + *Breaks:* numbers that collide, unnoticed: `FLW 25` already names two + different messages, and `GPL 200` sits in GPL's own range; and a check + that only one flow runs. ## Interface