include: use angle brackets for exported headers, and check it

[grandixximo]

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 under src/ 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.py reads 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.hh and hal.h are 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.

[grandixximo]

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 ;-)

[BsAtHome]

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.

[grandixximo]

Fair, I'll keep them as warning for now then.

[BsAtHome]

Fair, I'll keep them as warning for now then.

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.

[grandixximo]

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...

[grandixximo]

@BsAtHome The cross-dependency analysis you asked for. scripts/include-dep-report.py does it now, and there were three cycles.

emc/nml_intf and emc/tooldata was real and is cut in #4445. EMC_TOOL_STAT::operator= has no caller, so the store lookup and the include both go.

The five-way one inside libnml was not a dependency. Those six subdirectories have empty Submakefiles, every source is listed in libnml/Submakefile and links into one libnml.so, so libnml already is one library in its own directory and the report was drawing edges across a layout. #4446 makes it bucket by what the build links.

emc/motion and emc/tp is real, and I do not think an include rule should be what cuts it. motion.h embeds TP_STRUCT coord_tp by value, so a replacement tpmod cannot change that layout, and tp reads motmod's status struct going the other way. I deleted each include and rebuilt to see which carried weight: tc.c and sp_scurve.c need emcmot_status_t for planner_type, jerk and scurve_peak_scale, tp.c needs emcmot_hal_data_t, blendmath.c needs GET_TRAJ_PLANNER_TYPE() and the emcmotStatus global out of motion's private header. Only tp.c's axis.h was dead, and #4438 already removes it. Giving tp an interface is a design change, not include hygiene.

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.

[BsAtHome]

The fact that we treat some files differently is still a problem. That reminds me, have you checked rtapi/'s code? That is also building in a rather interesting way. The style check first becomes an issue when we have a tree that can support it without exceptions. Exceptions are a maintenance nightmare.

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).

[grandixximo]

@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 BUILD_SYS, RTAI and xenomai, so "the implementation of rtapi.h" is a configuration-dependent set and there is no table entry to write for it. Its own files already include their exported headers both ways, "rtapi.h" fifteen times and <rtapi.h> four.

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: SRCHEADERS at src/Makefile:401 lists 37 headers but only builds the HEADERS target list, while the cp recipes live in five per-directory *INCS lists plus three one-off rules. And headersclean removes $(HEADERS), derived from SRCHEADERS, so a header dropped from SRCHEADERS is never removed from include/; sincos.h is still sitting in mine after #4443. A stale export copy is exactly what an angled include resolves to.

Worth pursuing in that order?

[BsAtHome]

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.
The complicating issue is that the copying of the headers is one of the very first thing that needs to be done so all dependent rules will rebuild properly (the headers rule).

When we remove a header from being exported, then that is always a problem. You need a make clean and in that process remove everything from the include/ directory.

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.

[grandixximo]

Gets back to scrubbing...

[grandixximo]

@BsAtHome Rebased, and the exception table is gone. With #4448 and #4449 in, the check passes on master with INTERFACES empty, so I removed it rather than carry two entries that no longer fire. The rule is now one sentence: an exported header and anything in its directory keep the quoted form, everyone else takes the angled one.

One cost, stated plainly. Without the table a file beside an exported header may quote it, so the five emc/ini users #4448 just fixed are no longer held to the angled form; only far-directory users are. That gap closes on its own once exported headers live in a directory of their own, which is where you were heading anyway.

Also fixed a crash: the file list came from git ls-files and was then opened blind, so a tracked path missing from the working tree, deleted but uncommitted or mid-rebase, took the script down instead of being skipped. CI never sees that, a pre-commit hook does.

#4452 has the one-rule header export you asked for.

[grandixximo]

If we don't merge, need keep checking these by hand, the exclusions are gone, any more blockers?

[BsAtHome]

How does this track includes in a component that is not using #include but uses include "bla";?

[grandixximo]

It did not. Fixed: the check now reads include "bla"; above the ;; line as well, where halcompile takes its own include directive rather than the preprocessor's, and .icomp is in the suffix list too. Both forms in a component are flagged now, and the tree is still clean.

[BsAtHome]

This is an assumption that may not be true. At least, it is not enforced.

[grandixximo]

Correct, nothing enforces it. It is an assumption about layout rather than about includes, and #4461 removes the layout: the 37 exported headers move into src/include, which holds nothing else. A sibling include then has to name a file in that same directory, so the check can verify it instead of assuming it.

[BsAtHome]

This assumes that the directory is determining whether a quoted include is fine. That is also an assumption.

[grandixximo]

Also correct. That branch exists only because sources sit beside exported headers. After #4461 none do, so it has no instances left and goes, and what is left is one rule with no exceptions.

[BsAtHome]

If we don't merge, need keep checking these by hand, the exclusions are gone, any more blockers?

It is not about will/wont merge. It is about me having reservations that the code base is ready for this (besides my review comments).