Bug #8571

Makefile.master should not trust $PATH

Added by Rich Lowe 12 months ago. Updated 5 months ago.

Status:ClosedStart date:2017-08-09
Priority:NormalDue date:
Assignee:Rich Lowe% Done:


Category:tools - gate/build tools
Target version:-
Difficulty:Medium Tags:needs-triage


There are a small number of places in Makefile.master where we do not specify full paths. With true(1) this is probably not a big deal, but with echo(1) and install(1) this may matter. We should not be so trusting of $PATH. Especially in the case of install(1), where we're the practically the only user of the SysV version.

Related issues

Related to illumos gate - Bug #4163: Illumos build should not break if /usr/bin/install is installed Rejected 2013-09-30


#1 Updated by Rich Lowe 6 months ago

  • Related to Bug #4163: Illumos build should not break if /usr/bin/install is installed added

#2 Updated by Rich Lowe 6 months ago

The downside of this, is that in cases where make is not able to use its builtin shell 'mksh' and has to use a real shell, the builtin versions of echo and true will not be used if we have specified the full paths.

In the current source tree this is a vast minority of instances, though partly that's because people don't use $(ECHO) and $(TRUE) as they should. Even then in the case of echo, it is a tiny instance, and it seems to me that consistent behaviour of echo is more important.

I struggle to believe someone could implement an incompatible 'true', but also struggle to believe it'll make sufficient difference to warrant commentary and the lack of uniformity

#3 Updated by Rich Lowe 6 months ago

Casual experimentation shows that I'm wrong in the comment above. It does indeed make a noticeable difference to not have the built-in echo, and we should only fix INS.

This is less bad, correctness-wise, since it's the echo builtin to the shell used by make, which will either be /bin/sh, or explicit in the Makefile, so at least it's still going to be as expected (unless that shell lacks a builtin echo, in which case we've time-travelled).

#4 Updated by Electric Monk 6 months ago

  • Status changed from In Progress to Closed
  • % Done changed from 0 to 100

git commit 6af78b59cf0bb5ad194f2ab51b2302d8c45d2543

commit  6af78b59cf0bb5ad194f2ab51b2302d8c45d2543
Author: Richard Lowe <richlowe@richlowe.net>
Date:   2018-01-27T21:41:20.000Z

    8571 Makefile.master should not trust $PATH
    Reviewed by: Toomas Soome <tsoome@me.com>
    Reviewed by: Peter Tribble <peter.tribble@gmail.com>
    Reviewed by: Andy Fiddaman <omnios@citrus-it.co.uk>
    Approved by: Dan McDonald <danmcd@joyent.com>

#5 Updated by Electric Monk 6 months ago

git commit 6c9c1c0d3911e7534e0028fb6e2a09aa4609730d

commit  6c9c1c0d3911e7534e0028fb6e2a09aa4609730d
Author: Richard Lowe <richlowe@richlowe.net>
Date:   2018-01-28T15:27:38.000Z

    backout: 8571 Makefile.master should not trust $PATH (insufficient testing, broke bootstrap)

#6 Updated by Electric Monk 5 months ago

git commit c08555783b53af86d5998de8a9ea7c58d83d75c9

commit  c08555783b53af86d5998de8a9ea7c58d83d75c9
Author: Richard Lowe <richlowe@richlowe.net>
Date:   2018-02-12T10:56:22.000Z

    8571 Makefile.master should not trust $PATH
    Reviewed by: Andy Stormont <astormont@racktopsystems.com>
    Approved by: Hans Rosenfeld <hans.rosenfeld@joyent.com>

Also available in: Atom