include: use angle brackets for exported headers, and check it - #4375
include: use angle brackets for exported headers, and check it#4375grandixximo wants to merge 1 commit into
Conversation
|
Since I cleaned the tree we can enforce it and error the CI when mistakes are done, objections? Anyway Bertho won't let them pass ;-) |
004a0ad to
b3e6a46
Compare
|
BTW, I don't think we should enforce this in CI. At least not yet. I'd like to see more cleanup before we start enforcing. The cleanup may expose situations that require a second thought. |
|
Fair, I'll keep them as warning for now then. |
| INTERFACES = { | ||
| "src/emc/ini/inifile.hh": ( | ||
| "src/emc/ini/inifile.cc", | ||
| ), | ||
| "src/hal/hal.h": ( | ||
| "src/hal/hal_lib.c", | ||
| "src/hal/hal_lib_extra.c", | ||
| "src/hal/hal_lib_query.c", | ||
| ), | ||
| } | ||
|
|
There was a problem hiding this comment.
The cases here are a requirement that they use the local version. Building the library code is local.
The problem with this is that there are many exceptions and marginal calls with the current organization.
Try emc or motion. The attempt to extract some of the constants was only partly done. All of that needs a cleanup too.
That I need to rephrase. I don't think it should be checked in CI yet, at all. (see my added review comment) The code organization is still messy. That is something that needs fixing first. We need to have (all) the libraries in their own isolated directory and analyze the cross-dependencies. |
|
Ok, I see, I'll get on the cleaning first then, but I'm taking a break tomorrow, traveling with family, I'll be around a bit less... |
b3e6a46 to
d2b5eee
Compare
d2b5eee to
cf6931e
Compare
|
@BsAtHome The cross-dependency analysis you asked for.
The five-way one inside libnml was not a dependency. Those six subdirectories have empty
So one cycle left and it is one I would leave alone. Does that settle the gate, or is there something else you want first? This is rebased on master. |
|
The fact that we treat some files differently is still a problem. That reminds me, have you checked I still think we should split this PR. One for the actual include type changes that are warranted and then the style check we need to work on a bit more (IMO). |
Nothing catches the wrong form today, since both compile, and the quoted one silently resolves to whichever copy sits nearest. The check reads the SRCHEADERS list, so it follows whatever the build exports. Exported headers are left alone: they are copied to include/ and have to keep finding their siblings there. A header whose own directory also holds code that merely uses it needs its implementation named, inifile.hh and hal.h being those cases in tree today. Everywhere else being in the header's directory is enough. Findings are warnings by default and errors with --error, which is how CI runs it, and named files can be passed for use from a pre-commit hook.
cf6931e to
82ce52a
Compare
|
@BsAtHome Split done. #4448 carries the include changes, this one is the check alone. CI here fails until that merges, since the eight lines it fixes are exactly what the check flags. On rtapi. It is 25 exported headers, 4 private ones and 13 sources in one directory, and which sources build depends on So rather than enumerate exceptions I would rather remove the need for them: give exported headers a directory that holds nothing else, so "sits beside an exported header" and "is an exported header" become the same statement. That is your isolated-directory point applied to the header side, and the check then needs no exceptions at all. rtapi is both the worst case and the best proof. Two things I ran into while looking. The export set has two sources of truth: Worth pursuing in that order? |
|
The multiple copy recipes are the actual problem, I think. They should not copy locally, but add to one variable and one rule does the copying. When we remove a header from being exported, then that is always a problem. You need a The only alternative is to move everything that is exportable to the include directory permanently (no more copying). But I fear that it also would make things difficult at this state. We need to have all the proper interfaces in the includes and nothing else. That will take some more cleanup before we get to that point. |
|
Gets back to scrubbing... |
2 participants
This has bitten me many times in review, and it is a rule a script can check, so here is the script plus the tree made consistent with it.
The build copies every SRCHEADERS entry into
include/, so an exported header exists twice: the source undersrc/and the copy modules compile against. A quoted include searches the includer's own directory first, an angled include does not, so the two forms can reach different copies of the same header. Both compile, always, which is why it is easy to get wrong and invisible until somebody reads the diff.The implementation keeps quotes, wanting the source beside it rather than a stale export. Everything else is a user, builds out of tree where only the exported copy exists, and takes angle brackets.
20 includes change, no code.
scripts/include-style-check.pyreads the SRCHEADERS list, so it follows what the build exports rather than a list of its own. Findings are warnings by default and errors with--error, which is how the cppcheck job runs it. Named files can be passed, which suits a pre-commit hook. It annotates the offending lines when it runs in CI, so the next case shows up on the diff before a reviewer has to write it out.One judgment call sits in the script rather than in the rule: a header whose own directory also holds code that only uses it needs its implementation named.
inifile.hhandhal.hare those cases in tree today.Full build clean. The check reports nothing on the result, and reports each of the 20 again when they are put back one at a time.