Page MenuHomeFreeBSD

Start to split out the really x86 specific NOTES from the global notes file. Start with COMPAT_43, since it's really only relevant to x86.
ClosedPublic

Authored by imp on Sat, Aug 10, 12:41 AM.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

imp created this revision.Sat, Aug 10, 12:41 AM
imp added a reviewer: jhb.Sat, Aug 10, 12:42 AM
imp added inline comments.
sys/x86/conf/NOTES
12 ↗(On Diff #60628)

This comment is so FreeBSD 3.x and likely needs to be modernized

jhb added inline comments.Sat, Aug 10, 12:44 AM
sys/conf/NOTES
339 ↗(On Diff #60628)

It would be good to copy the header bit over as well. I think in the MD NOTES files we keep the header blocks and try to keep the options sorted in the same relative order as MI NOTES.

sys/conf/makeLINT.mk
15 ↗(On Diff #60628)

I thought we'd just hack this file instead perhaps to add something like:

NOTES=
.if ${TARGET} == "amd64" || ${TARGET} == "i386"
NOTES+= x86/conf/NOTES
.endif

In particular, you could replace that with something using TARGET_CPUARCH in the future.

sys/x86/conf/NOTES
12 ↗(On Diff #60628)

Yeah, that's orthogonal, but probably it should say something like "this is only needed for legacy FreeBSD 1.x i386 a.out binaries"

imp added inline comments.Sat, Aug 10, 1:07 AM
sys/conf/NOTES
339 ↗(On Diff #60628)

ok

sys/conf/makeLINT.mk
15 ↗(On Diff #60628)

Yea, we could in the future, but now we can't. And we'd need to add back the ${.CURDIR}/NOTES too, which makes it too messy, imho, which is why I opted just for the default value.

imp added a comment.Sat, Aug 10, 4:19 PM
sys/conf/makeLINT.mk
15 ↗(On Diff #60628)

In addition, I think that the current state of the values of MACHINE_CPUARCH would mean that using it here would be hard. Based on all this, I think my approach is simpler, but we can revisit once we have MACHINE_CPUARCH=x86 for i386/amd64 in place.

imp updated this revision to Diff 60637.Sat, Aug 10, 4:26 PM

changes per jhb review, except the ifdef x86 in makeLINT.mk

jhb added inline comments.Mon, Aug 12, 4:54 PM
sys/conf/makeLINT.mk
15 ↗(On Diff #60628)

Just to be clear, we wouldn't have to add back .CURDIR/NOTES, you'd keep the existing line and have a += under it:

NOTES=	${.CURDIR}/../../conf/NOTES ${.CURDIR}/NOTES
.if ${TARGET} == "amd64" || ${TARGET} == "i386"
NOTES+= ${.CURDIR}/../../x86/conf/NOTES
.endif

Either way is fine I guess, but I feel like this is clearer that we are just adding a new file instead of duplicating the default value of NOTES.

jhb added a comment.Mon, Aug 12, 4:58 PM

OTOH, I wonder if what we might not want instead is a 'NOTES_EXTRA'. You would modify makeLINT.mk to do something like this:

NOTES=	${.CURDIR}/../../conf/NOTES ${.CURDIR}/NOTES ${NOTES_EXTRA}

We might find that there are some other large "features" we might want to split out into an MI NOTES but only enable them for certain architectures.

An even simpler way perhaps might be to change makeLINT.mk to just use 'NOTES+=' instead of 'NOTES=', then you could change amd64/Makefile and i386/Makefile
to just have:

NOTES=${.CURDIR}/../../x86/conf/NOTES

And use '+=' in makeLINT.mk. Perhaps that is the simplest of all.

imp added a comment.Mon, Aug 12, 5:11 PM
In D21203#461464, @jhb wrote:

OTOH, I wonder if what we might not want instead is a 'NOTES_EXTRA'. You would modify makeLINT.mk to do something like this:

NOTES=	${.CURDIR}/../../conf/NOTES ${.CURDIR}/NOTES ${NOTES_EXTRA}

We might find that there are some other large "features" we might want to split out into an MI NOTES but only enable them for certain architectures.
An even simpler way perhaps might be to change makeLINT.mk to just use 'NOTES+=' instead of 'NOTES=', then you could change amd64/Makefile and i386/Makefile
to just have:

NOTES=${.CURDIR}/../../x86/conf/NOTES

And use '+=' in makeLINT.mk. Perhaps that is the simplest of all.

True. Does the order matter? I guess it doesn't.

imp updated this revision to Diff 60682.Mon, Aug 12, 5:23 PM

update per jhb's suggestions

jhb accepted this revision.Mon, Aug 12, 5:57 PM
This revision is now accepted and ready to land.Mon, Aug 12, 5:57 PM