Page MenuHomeFreeBSD

Replace dot-dot relative pathing with SRCTOP/:H make manipulation where possible
ClosedPublic

Authored by ngie on Jan 17 2017, 4:28 AM.

Details

Summary

Replace dot-dot relative pathing with SRCTOP/:H make manipulation where possible

I log my buildworlds and buildkernels with script, and have noticed
a lot of "../" scrolling by over time -- I'd prefer to get rid of
these so it writes less unnecessary characters to disk and slows down
my terminal output less

MFC after: 1 week
Sponsored by: Dell EMC Isilon

Test Plan

Before patch:

% make cleandir -j12; script all.ts time make all -j12 MK_TESTS=no
...
objcopy --strip-debug --add-gnu-debuglink=libc.so.7.debug  libc.so.7.full libc.so.7
      111.55 real       280.00 user        39.12 sys
% wc -c all.ts
 5118811 all.ts
%

Script done, output file is all.ts

After patch:

% make cleandir -j12; script all.ts time make all -j12 MK_TESTS=no
...
objcopy --strip-debug --add-gnu-debuglink=libc.so.7.debug  libc.so.7.full libc.so.7
      111.70 real       279.52 user        39.50 sys

Script done, output file is all.ts
% wc -c all.ts                                                                                                                                                           
 4836944 all.ts
%

(and that savings is with just regular make output -- I'm not using -d at all)

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

ngie updated this revision to Diff 24100.Jan 17 2017, 4:28 AM
ngie retitled this revision from to Replace dot-dot relative pathing with SRCTOP/:H make manipulation where possible.
ngie updated this object.
ngie edited the test plan for this revision. (Show Details)
ngie added subscribers: marcel, bdrewery.
ngie updated this revision to Diff 24101.Jan 17 2017, 4:30 AM
  • Remove unrelated stdio.h diff
  • Add lib/libc/resolv/Makefile.inc diff

Overall I like it. A few questions about :H vs ${SRCTOP}.

lib/libc/Makefile
39 ↗(On Diff #24101)

Should this one be ${SRCTOP}/include? It looks like we intend to allow compiling libc from a different source location (i.e. LIBC_SCRTOP?= so referencing the system include via ${LIBC_SRCTOP} seems less desirable.

lib/libc/md/Makefile.inc
3 ↗(On Diff #24101)

this one arguably should be ${SRCTOP}-based

ngie updated this revision to Diff 24145.Jan 18 2017, 5:18 AM

Update based on feedback from emaste

ngie marked 2 inline comments as done.Jan 18 2017, 5:23 AM
ngie added a subscriber: sjg.
ngie added inline comments.
lib/libc/Makefile
39 ↗(On Diff #24101)

Agreed. r262722 seems like it's done in the vein of standardizing SRCTOP/OBJTOP, but for a very specific purpose that could instead be solved via SRCTOP universally now.

I CCed marcel though, just to be safe. I'll add sjg as well.

emaste added inline comments.Jan 19 2017, 8:09 PM
lib/libc/iconv/Makefile.inc
25 ↗(On Diff #24145)

Perhaps this should be based on ${SRCTOP} too, in which case (with my other comment just added) we'll have no ${LIBC_SRCTOP}/.. left.

lib/libc/stdtime/Makefile.inc
4–5 ↗(On Diff #24145)

Is this right? The path is lib/libc/locale no?

I don't want to assume I remember all the details with respect to compiling libc from a different locations, but what I recall is that it's only about building libc using a different makefile that lives in a different directory and thus having the object files go to a different object directory. Thus any mention of ${.CURDIR} had to be replaced with LIBC_SRCTOP and LIBC_SRCTOP was effectively set to ${SRCTOP}/lib/libc.

As such, I think it's fine to replace ${LIBC_SRCTOP:H} with ${SRCTOP}/lib, ${LIBC_SRCTOP:H:H} with ${SRCTOP}, and consequently any paths that start with one of those.

I recall that -I options needed to be tweaked, but those were prepended to CFLAGS from within the alternate makefile.

My general advise therefore is to not change semantics. Make it a mechanical bug-for-bug compatible change.
Thus replace ${LIBC_SRCTOP}/../locale with ${LIBC_SRCTOP:H}/locale or alternatively with ${SRCTOP}/lib/locale...

sjg added a comment.Jan 19 2017, 10:10 PM

General point; using :H is preferable to ../ if only to speed up filesystem access.

As several comments above have already made reasonably clear, much of the ../../ references in the FreeBSD makefiles can be replaced with SRCTOP relative references, this not only makes the intend of the makefiles clearer but also aids tree refactoring.

AFAIK the prevelant use of ../.. references is largely a consequence of .CURDIR and .OBJDIR traditionally being the only known reference points.
With the advent of .PARSEDIR and the ability to reliably define SRCTOP, this is no longer the case, and considerable cleanup of the makefiles is possible.

My general advise therefore is to not change semantics. Make it a mechanical bug-for-bug compatible change.
Thus replace ${LIBC_SRCTOP}/../locale with ${LIBC_SRCTOP:H}/locale or alternatively with ${SRCTOP}/lib/locale...

I think it's probably reasonable to make this a mechanical change and not change semantics, but then I think we should either fix or drop the locale path first in an initial commit. lib/locale doesn't exist, so having it in .PATH doesn't do any good.

ngie marked an inline comment as done.Jan 20 2017, 1:47 AM

My general advise therefore is to not change semantics. Make it a mechanical bug-for-bug compatible change.
Thus replace ${LIBC_SRCTOP}/../locale with ${LIBC_SRCTOP:H}/locale or alternatively with ${SRCTOP}/lib/locale...

I think it's probably reasonable to make this a mechanical change and not change semantics, but then I think we should either fix or drop the locale path first in an initial commit. lib/locale doesn't exist, so having it in .PATH doesn't do any good.

Agreed. I'll wait for you to commit your patch before I commit my larger one.

ngie edited the test plan for this revision. (Show Details)Jan 20 2017, 1:47 AM
ngie edited the test plan for this revision. (Show Details)
In D9207#191296, @sjg wrote:

General point; using :H is preferable to ../ if only to speed up filesystem access.
As several comments above have already made reasonably clear, much of the ../../ references in the FreeBSD makefiles can be replaced with SRCTOP relative references, this not only makes the intend of the makefiles clearer but also aids tree refactoring.
AFAIK the prevelant use of ../.. references is largely a consequence of .CURDIR and .OBJDIR traditionally being the only known reference points.
With the advent of .PARSEDIR and the ability to reliably define SRCTOP, this is no longer the case, and considerable cleanup of the makefiles is possible.

I agree 1000%.

emaste accepted this revision.Jan 20 2017, 2:34 AM
emaste added a reviewer: emaste.

LGTM with the changes discussed above

This revision is now accepted and ready to land.Jan 20 2017, 2:34 AM
ngie updated this revision to Diff 24222.Jan 20 2017, 3:13 AM
ngie edited edge metadata.

Apply review comments

This revision now requires review to proceed.Jan 20 2017, 3:13 AM
ngie marked an inline comment as done.Jan 20 2017, 3:15 AM
emaste accepted this revision.Jan 20 2017, 3:18 AM
emaste edited edge metadata.
This revision is now accepted and ready to land.Jan 20 2017, 3:18 AM
This revision was automatically updated to reflect the committed changes.