Prefer ${SRCTOP}/foo over ${.CURDIR}/../../foo and ${SRCTOP}/usr.bin/foo over ${.CURDIR}/../foo for paths in Makefiles.
ClosedPublic

Authored by imp on Mar 9 2017, 6:05 PM.

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.
imp created this revision.Mar 9 2017, 6:05 PM
imp edited the summary of this revision. (Show Details)Mar 9 2017, 6:06 PM
imp edited the test plan for this revision. (Show Details)
imp added a reviewer: bdrewery.
bdrewery accepted this revision.Mar 9 2017, 6:16 PM
This revision is now accepted and ready to land.Mar 9 2017, 6:16 PM
brooks accepted this revision.Mar 10 2017, 2:38 AM
brooks added a subscriber: brooks.

LGTM

julian added a subscriber: julian.Mar 10 2017, 2:52 AM

just my $0.02

usr.bin/bzip2/Makefile
3 ↗(On Diff #26099)

choosing just the first of these to respond to but this applies to ALL the changes here:
In my experience this should be done with a /usr/src/Makefile.paths. or /usr/src/srcpaths.mk
this should be included by some of the .mk files.
All these paths should be in that file in the form of SRCPATH_BZIP2=${SRCTOP}/contrib/bzip2
that way:
1/ when you pull some freebsd code for another project you only have one place to change
2/ you can move things or redefine the way this is done in one place

Also the definition of all these could include ${FOO:tA} (I hope I got that right). we should import gmake's $(realpath..) and $(abspath...) which is much more usable.

bdrewery added inline comments.Mar 10 2017, 2:56 AM
usr.bin/bzip2/Makefile
3 ↗(On Diff #26099)

The first is an interesting idea but it's a bit out of scope of this work.
The second, of using :tA everywhere, is understood for why it's wanted but it would slow the build down quite a lot. Our realpath(3) is horribly slow.

bdrewery added inline comments.Mar 10 2017, 2:57 AM
usr.bin/bzip2/Makefile
3 ↗(On Diff #26099)

I always hit enter too fast. It was so slow that we made bmake cache the results *per process*. So reusing the same :tA in 1 make execution is fast, but across the entire tree if we applied :tA all over the place it would just smash the namecache and create a ton of extra contention there.

imp added a comment.Mar 10 2017, 3:04 AM

just my $0.02

Nice two cents. Not going to do it since it is beyond the scope of this work, but it is a nice idea down the road. Similar to libnames.mk, it would make things better. But there's a lot of them in the tree, so that's outside the scope.

Torn on the FOO:tA thing since isn't needed, would slow things down and SRCTOP is already an absolute path so it wouldn't change anything. But that's a thought for another time.

In D9932#205427, @imp wrote:

just my $0.02

Nice two cents. Not going to do it since it is beyond the scope of this work, but it is a nice idea down the road. Similar to libnames.mk, it would make things better. But there's a lot of them in the tree, so that's outside the scope.

just an idea for sometime

Torn on the FOO:tA thing since isn't needed, would slow things down and SRCTOP is already an absolute path so it wouldn't change anything. But that's a thought for another time.

I think I overstate it with "everywhere: I think "in selected places" might be a better term. We use the gmake equivalent $(abspath $(somepath)) a lot and it makes things a lot more readable. As for the speed of it, whatever gmake does doesnt seem to take so long as I've never noticed a slowdown.

imp added a comment.Mar 10 2017, 4:35 AM

I think I overstate it with "everywhere: I think "in selected places" might be a better term. We use the gmake equivalent $(abspath $(somepath)) a lot and it makes things a lot more readable. As for the speed of it, whatever gmake does doesnt seem to take so long as I've never noticed a slowdown.

In what way. What things were flakey and how did this fix them?

imp updated this revision to Diff 26124.Mar 10 2017, 6:25 AM
imp edited edge metadata.
  • Move /etc/ to SRCTOP
  • Convert include over to SRCTOP
  • Fix two CURDIR references in comments that should be SRCTOP
  • Make rescue use SRCTOP
  • Fold with usr.bin
  • Convert gnu/lib to using SRCTOP
  • gnu/usr.bin SRCTOP
This revision now requires review to proceed.Mar 10 2017, 6:25 AM
This revision was automatically updated to reflect the committed changes.