The-OpenROAD-Project / The-OpenROAD-Project/OpenROAD
Making all files `#include`-ed relative to project root
Nobody has claimed this yet.
- Dominant language
- Verilog
- Stars
- 3.1k
- Forks
- 1k
- Avg merge
- 2d 23h
- Merged PRs (30d)
- 136
Description
(all hand written, no AI involved)
TL;DR:
Instead of using aliased header names, use fully qualified names from project root
-#include "db_sta/dbNetwork.hh"
+#include "src/dbSta/include/db_sta/dbNetwork.hh"
Motivation
OpenROAD consists of various submodules that each have their own sources and headers. Traditionally, these have been structured in a way that a lot of -I allow to simply include a short alias db_sta/dbNetwork.hh instead of the fully qualified one src/dbSta/include/db_sta/dbNetwork.hh.
This also made it slightly easier with the first build system (cmake) to get some perceived isolation. The now also maintained bazel build system allows to have strict visibility of internal and external headers, so this is not needed anymore and can allow for a much more easy to navigate project structure. Bazel projects strongly encourage the use of fully qualified headers relative to project root.
Using these paths would also allow an unambiguous use of a header, allows the user to easier see where something is coming from and also reduces the number of -I-paths that we have to pass to the compiler, reducing IO.
Downsides
Right now, the source layout contains multiple redundant path elements (e.g. above dbSta and db_sta, and some meaningless 'include/' element as well), so using the full project-relative paths are somewhat of a mouth-full: src/dbSta/include/db_sta/dbNetwork.hh.
However, it will also allow a much easier refactoring of the whole project structure once we don't need cmake anymore -- because if the names are fully qualified, there is not ambiguity about renaming them.
(in the future I'd imagine this particular header should probably be openroad/db_sta/dbNetwork.hh
Feasibility
In bant 0.2.9, there is a new feature that allows to find all unambiguous header names and provide the mapping to the fully qualified canonical name. So yes, a bulk rename is possible (tested locally, everything compiles on bazel; for cmake there is probably one toplevel -I needed)
Merge conflicts ? Avoid by staggered changes.
Problem with a bulk rename will be that it pretty much has to touch every file. There are a bunch of files at any given time that are open in pull requests.
My suggestion is to filter out all files that are currently open in pull requests and do the bulk rename on the remaining ones. As the open pull requests get merged, repeat until all files are handled. This will bring minimal disruption, and will result in the same outcome, but might take a few weeks to finish.
Since the project still retains all the -I and includes = [...] attributes, compilation will work in this mixed environment.
Afterwards: clean up includes = [ ... ]
Now that every include is relative to project root (in the main project; we leave the headers in the sta sub-project untouched), we now can remove all the includes = [...] attributes in cc_library(), reducing the number of -I needed.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reviewing the bant 0.2.9 srcs-canonical feature and its generated mapping, then inspect the existing Bazel and CMake include paths. Track files not covered by the staggered migration and verify Bazel and CMake compilation after each batch. Done means main-project includes use project-root-relative paths, while STA headers remain unchanged; clean up cc_library() includes afterward.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- build-system
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100