Details

Reviewers

markj
brooks

Commits

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

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 Passed

Unit

No Test Coverage

Build Status

Buildable 42939
Build 39827: arc lint + arc unit

Event Timeline

imp created this revision.Oct 14 2021, 5:22 PM

Herald added a subscriber: jonathan. · View Herald TranscriptOct 14 2021, 5:22 PM

imp requested review of this revision.Oct 14 2021, 5:22 PM

Harbormaster completed remote builds in B42140: Diff 96873.Oct 14 2021, 5:22 PM

imp edited the test plan for this revision. (Show Details)Oct 14 2021, 5:24 PM

imp added a reviewer: markj.

imp added inline comments.Oct 14 2021, 5:43 PM

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

In D32498#733144, @markj wrote:

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.

In D32498#733162, @brooks wrote:

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.

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.

Harbormaster completed remote builds in B42145: Diff 96887.Oct 15 2021, 4:17 AM

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

Harbormaster completed remote builds in B42146: Diff 96888.Oct 15 2021, 4:26 AM

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

Harbormaster completed remote builds in B42148: Diff 96915.Oct 15 2021, 3:47 PM

imp edited the summary of this revision. (Show Details)Oct 15 2021, 3:49 PM

this time with goodfiles.inc <sigh>

Harbormaster completed remote builds in B42149: Diff 96916.Oct 15 2021, 3:50 PM

hack for phab

Harbormaster completed remote builds in B42150: Diff 96917.Oct 15 2021, 3:51 PM

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.

Harbormaster completed remote builds in B42151: Diff 96918.Oct 15 2021, 4:19 PM

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

Harbormaster completed remote builds in B42152: Diff 96919.Oct 15 2021, 5:15 PM

.PHONY fix

Harbormaster completed remote builds in B42153: Diff 96920.Oct 15 2021, 5:20 PM

imp added inline comments.Oct 15 2021, 6:05 PM

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

Herald added a subscriber: emaste. · View Herald TranscriptNov 23 2021, 5:48 PM

Harbormaster completed remote builds in B42939: Diff 98880.Nov 23 2021, 5:48 PM

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
1139	moving it here addresses a now-vanished comment from @brooks that we should fail early, not later

LTGM

This revision is now accepted and ready to land.Nov 23 2021, 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.Nov 24 2021, 10:20 AM

Harbormaster completed remote builds in B42954: Diff 98946.Nov 24 2021, 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
19	Was it intentional to leave this commented out?

This revision is now accepted and ready to land.Nov 25 2021, 2:21 PM

markj added inline comments.Nov 25 2021, 2:22 PM

tools/build/test-includes/Makefile
16	You might add a comment explaining why _*.h are excluded.

imp added inline comments.Nov 25 2021, 7:37 PM

tools/build/test-includes/Makefile
16	Good idea.
19	It was waiting for another patch to go into the tree. I've committed that and uncommented locally after my last update.

Closed by commit rGdd55767b86bc: Test various header files to ensure they can be included by themselves. (authored by imp). · Explain WhyDec 15 2021, 3:26 AM

This revision was automatically updated to reflect the committed changes.

imp added a commit: rGdd55767b86bc: Test various header files to ensure they can be included by themselves..

imp added a reverting change: rG6a2b72bf1a88: Remove test-includes build.Dec 15 2021, 4:43 AM

I had to back out the automatic testing because this is built at a time we have no libraries yet, so test-includes.c can't create a binary.
A number of options are possible, and I'll ponder them over the coming days.
My universe, which wasn't completely clean (since I use meta mode) didn't catch this possible issue.

In D32498#756273, @imp wrote:

I had to back out the automatic testing because this is built at a time we have no libraries yet, so test-includes.c can't create a binary.
A number of options are possible, and I'll ponder them over the coming days.
My universe, which wasn't completely clean (since I use meta mode) didn't catch this possible issue.

Hmm, maybe switch to using bsd.lib.mk so there are no dependencies or will we still need csu for that? Alternatively, it could be that we just need a target for ${PROG} that just calls touch.

Since this has been out for a while now it is time for fall-out observations.

Why exclude underscore headers? Since they're the building blocks of other headers they need to pass this test, otherwise they cause other headers to fail.

This test needs two passes. I think there should be a second pass where _KERNEL is defined. This aids driver writers since it ensures kernel headers also follow these helpful rules.

Shouldn't headers in machine/ also be tested?

Wouldn't a single find | sort be better than the complicated construct with ls and multiple sorts? I understand the attraction of keeping sys first, but the file is nominally supposed to be auto-generated and build speed is more important.

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 98880

Makefile

Makefile.inc1

tools/build/test-includes/Makefile

tools/build/test-includes/badfiles.inc

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 98880

Makefile

Makefile.inc1

tools/build/test-includes/Makefile

tools/build/test-includes/badfiles.inc

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

Revision Contents
Changeset List