Page MenuHomeFreeBSD

Test various header files to ensure they can be included by themselves.
AcceptedPublic

Authored by imp on Oct 14 2021, 5:22 PM.

Details

Reviewers
markj
brooks
Summary

A number of header files in sys/* have, going back to 7th Edition Unix
in 1979, reqiured other files (like sys/types.h) to compile. Likewise
the 4BSD networking code has had prerequisites. However, going back to
around the turn of the century, other systems have made them be
independently includable (wide-spread header include protection
post-dates 7th edition Unix by maybe 4 or 5 years judging from netnews
sources). Start down the path of making them all independently
includable by creating this test that fails buildworld when they are
not.

The file 'goodfiles.inc' contains a list of the currently working files
that can be included w/o any prerequisites. As files are fixed, 'make
goodfiles.inc' should be re-run to regenerate the list. That target
should never be run when files have broken.

Test Plan

I know this isn't actually part of buildworld
And I'd be keen on knowing whether or not an extra file with all the header files would be a good idea
As well as creating an actual dummy program that might make this a little easier.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 42954
Build 39842: arc lint + arc unit

Event Timeline

imp requested review of this revision.Oct 14 2021, 5:22 PM
imp added a reviewer: markj.
usr.bin/hdrtest/Makefile
180 ↗(On Diff #96873)

This is the bug I'm fixing :)

I know this isn't actually part of buildworld
And I'd be keen on knowing whether or not an extra file with all the header files would be a good idea
As well as creating an actual dummy program that might make this a little easier.

So as developers add new headers, we want to ensure that they don't have external dependencies. With the scheme in the proposed diff, they have to manually add their header to this file. I think a script (or makefile I guess) which tests every single header that we install and compares failures against a known-bad list would require the least maintenance. I'm not quite sure what an "extra file with all of the header files" would be for - can't the makefile do something like "HDRS!=find <directories> -name \*.h" and test each one? Or is that too hacky?

I think having such a test as a part of buildworld is a good idea.

usr.bin/hdrtest/Makefile
2 ↗(On Diff #96873)

I think the review description would be useful as a comment here (or anywhere).

I know this isn't actually part of buildworld
And I'd be keen on knowing whether or not an extra file with all the header files would be a good idea
As well as creating an actual dummy program that might make this a little easier.

So as developers add new headers, we want to ensure that they don't have external dependencies. With the scheme in the proposed diff, they have to manually add their header to this file. I think a script (or makefile I guess) which tests every single header that we install and compares failures against a known-bad list would require the least maintenance. I'm not quite sure what an "extra file with all of the header files" would be for - can't the makefile do something like "HDRS!=find <directories> -name \*.h" and test each one? Or is that too hacky?

I'm torn. It sure would be nice to have a way to generate this list. We could mark the files with /* INDEPENDENT */ at the top
which would also a nice hint to the reader and grep for it...

I think having such a test as a part of buildworld is a good idea.

Yea, me too... Too easy to break.

Thinking about @markj's idea, a BADHDRS variable to filter out headers from a generated list wouldn't be hard to maintain.

Thinking about @markj's idea, a BADHDRS variable to filter out headers from a generated list wouldn't be hard to maintain.

Today, the filter out list is larger than the filter in list. So I'd really rather keep the shorter list until it's > 50%. In the mean time, I'd like to agree on some way of marking the 'working' files and grep for them instead of either an opt-in or an opt-out list. If we can get *THAT* into place, then it would be better than either.

Also, I'm just going to leave this in usr.bin. includes isn't quite right, tests isn't quite right. This isn't quite right either, but seems less wrong than all those other places, but I'll sleep on it to see if there's any other less-bad places for this.

imp edited the test plan for this revision. (Show Details)

Pull in most of brooks' suggested changes
Also augment it a little with cleaning the generated files
Still not sure how the target ${.OBJDIR}/sys gets created, but special code to
create it seems unnecessary.

Better comment... Will harmonize it and the commit message.

If you're using a bin directory I'd use bin suggest bin as it's likely to be hit earlier.

I don't find the logic of one list being longer than the other being compelling. Picking them all and filtering the broken ones prevents new broken files form being introduced. If you don't want to look at the list add a Makefile.HDRS and .include it (this has spilled into the realm where I'd start to do that).

usr.bin/hdrtest/Makefile
187 ↗(On Diff #96888)

I'd put this next to SRCS above

I'm thinking .include with a target to regenerate the file... the list would keep things working. Regenerating when you fix something would be easy and thus the list would only grow. No need for marking or filtering.

updates based on suggestions so far: move to a goodfiles list that's updated by
a makefile target. This makes maintaining the list trivial and so long as we run
the target only when hdrtest is buildable, the list will only ever increase in
size (absent those rare cases we remove files from /usr/include).

this time with goodfiles.inc <sigh>

Note:

bin/hdrtest/Makefile
33 ↗(On Diff #96917)

I'm half tempted to make this depend on ${PROG} -- comments?

Add net*/*.h too
We're at ~250/650 being stand alone in these directories.

Move to opt-out rather than opt-in list.

bin/hdrtest/Makefile
33 ↗(On Diff #96917)

This concern is addressed by the transition from good -> bad.

I'm going to rework this a little... I'm going to move what's here with some tweaks to tools/build/test-includes and build that just after the make includes pass...
Will also create a test-includes top-level target so we can test it w/o doing a full buildworld once the pump is primed (that is, the build has proceeded through at
least the part in make _installs where this test failed :).

update now that it seems to work

Note: Need to document test-includes in Makefile and Makefile.inc headers (and maybe build(9)), so I know that, but other issues with this shift in approach are fair game :)

Also of note: two new files have been added to the bad list....

Makefile.inc1
1138

moving it here addresses a now-vanished comment from @brooks that we should fail early, not later

This revision is now accepted and ready to land.Tue, Nov 23, 10:22 PM

Running tinderbox on this now.

Don't test _*.h files
minor other tweaks found while testing

This revision now requires review to proceed.Wed, Nov 24, 10:20 AM

This looks good!

If you were planning on using the review description as the commit message, this part needs to be updated:

The file 'goodfiles.inc' contains a list of the currently working files
that can be included w/o any prerequisites. As files are fixed, 'make
goodfiles.inc' should be re-run to regenerate the list. That target
should never be run when files have broken.
tools/build/test-includes/Makefile
18

Was it intentional to leave this commented out?

This revision is now accepted and ready to land.Thu, Nov 25, 2:21 PM
tools/build/test-includes/Makefile
15

You might add a comment explaining why _*.h are excluded.

tools/build/test-includes/Makefile
15

Good idea.

18

It was waiting for another patch to go into the tree. I've committed that and uncommented locally after my last update.