Fix INSTALL_AS_USER
ClosedPublic
Actions

Authored by vangyzen on May 19 2017, 1:56 AM.

Details

Reviewers

bdrewery
sjg

Commits

rS319356: MFC r319020
rS319020: Fix INSTALL_AS_USER

Summary

Move INSTALL_AS_USER into bsd.init.mk to maximize the chance that it has final authority over fooOWN and fooGRP.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 9435
Build 9889: arc lint + arc unit

Event Timeline

vangyzen created this revision.May 19 2017, 1:56 AM

Herald added a subscriber: badger. · View Herald TranscriptMay 19 2017, 1:56 AM

Harbormaster completed remote builds in B9368: Diff 28544.May 19 2017, 1:56 AM

vangyzen added a reviewer: sjg.May 19 2017, 1:58 AM

These changes are wrong. In the normal case, without INSTALL_AS_USER, the group is not getting set correctly.

I must be missing something subtle, because there are several other commands that set BINOWN=root (for example) but worked with INSTALL_AS_USER without any changes. For example, traceroute didn't work, but traceroute6 did.

Do I need to test MK_INSTALL_AS_USER in all of these Makefiles instead of using ?= for assignment?

Now I see that traceroute/Makefile includes src.opts.mk and therefore bsd.own.mk before setting BINOWN, and traceroute6/Makefile does not. All of the Makefiles I changed do this.

What is the right fix? Should we move the INSTALL_AS_USER code out of bsd.own.mk and into a file included only by bsd.{lib,prog}.mk? Or should all of these Makefiles test MK_INSTALL_AS_USER before setting xyz{OWN,GRP}?

This level of churn is probably not desirable.
esp since IIRC there are some that want to get rid of INSTALL_AS_USER

Moving the INSTALL_AS_USER logic out of bsd.own.mk would make sense since that location is problematical
given the early include via src.opts.mk
src.init.mk might make sense?

In D10810#224258, @sjg wrote:

This level of churn is probably not desirable.

Agreed, and it probably won't get committed...especially since it doesn't work. =-O

esp since IIRC there are some that want to get rid of INSTALL_AS_USER

I would very much like to keep it. Do you know off-hand who wants to kill it? I suppose I'll ask on a ML.

Moving the INSTALL_AS_USER logic out of bsd.own.mk would make sense since that location is problematical
given the early include via src.opts.mk
src.init.mk might make sense?

I assume you meant bsd.init.mk. That looks like a good place, at least for avoiding the current problem. @bdrewery, what do you think?

I'm currently testing a change that moves the INSTALL_AS_USER logic into bsd.init.mk.

Relocate INSTALL_AS_USER logic

Move INSTALL_AS_USER into bsd.init.mk to maximize the chance that
it has final authority over fooOWN and fooGRP.

Herald added a subscriber: emaste. · View Herald TranscriptMay 22 2017, 1:24 PM

Harbormaster completed remote builds in B9435: Diff 28667.May 22 2017, 1:24 PM

This works, in both the normal case and INSTALL_AS_USER.

vangyzen edited the summary of this revision. (Show Details)May 22 2017, 2:36 PM

vangyzen mentioned this in D10862: Fix INSTALL_AS_USER with external nsswitch databases.May 23 2017, 8:21 PM

sjg accepted this revision.May 27 2017, 10:07 PM

This revision is now accepted and ready to land.May 27 2017, 10:07 PM

Closed by commit rS319020: Fix INSTALL_AS_USER (authored by vangyzen). · Explain WhyMay 28 2017, 12:43 AM

This revision was automatically updated to reflect the committed changes.

bdrewery added inline comments.Jul 10 2017, 4:57 PM

head/share/mk/bsd.init.mk
16–19 ↗	(On Diff #28919)	You moved the logic from bsd.own.mk (more coverage) to bsd.init.mk (less coverage) and it is right after an include of bsd.own.mk. I don't quite understand why. It breaks INSTALL_AS_USER in etc/Makefile since it (arguably incorrectly) uses _uid before including bsd.prog.mk/bsd.init.mk

bdrewery added inline comments.Jul 10 2017, 5:03 PM

head/share/mk/bsd.init.mk

16–19 ↗

(On Diff #28919)

This fixes it but I'm not sure I like it. Makefiles may have a valid need to know what user is being installed as without funky hacks.

diff --git etc/Makefile etc/Makefile
index 7eadb1bba99b..2f94d7806c42 100644
--- etc/Makefile
+++ etc/Makefile
@@ -342,19 +342,6 @@ distribution:
 
 MTREE_CMD?=	mtree
 
-.if ${MK_INSTALL_AS_USER} == "yes" && ${_uid} != 0
-MTREE_FILTER= sed -e 's,\([gu]\)name=,\1id=,g' \
-	-e 's,\(uid=\)[^ ]* ,\1${_uid} ,' \
-	-e 's,\(gid=\)[^ ]* ,\1${_gid} ,' \
-	-e 's,\(uid=\)[^ ]*$$,\1${_uid},' \
-	-e 's,\(gid=\)[^ ]*$$,\1${_gid},' 
-.else
-MTREE_FILTER= cat
-.if !defined(NO_FSCHG)
-MTREE_FSCHG=	-i
-.endif
-.endif
-
 MTREES=		mtree/BSD.root.dist		/		\
 		mtree/BSD.var.dist		/var		\
 		mtree/BSD.usr.dist		/usr		\
@@ -467,3 +454,16 @@ etc-examples: etc-examples-install
 	    DESTDIR=${DESTDIR}${SHAREDIR}/examples
 
 .include <bsd.prog.mk>
+
+.if ${MK_INSTALL_AS_USER} == "yes" && ${_uid} != 0
+MTREE_FILTER= sed -e 's,\([gu]\)name=,\1id=,g' \
+	-e 's,\(uid=\)[^ ]* ,\1${_uid} ,' \
+	-e 's,\(gid=\)[^ ]* ,\1${_gid} ,' \
+	-e 's,\(uid=\)[^ ]*$$,\1${_uid},' \
+	-e 's,\(gid=\)[^ ]*$$,\1${_gid},' 
+.else
+MTREE_FILTER= cat
+.if !defined(NO_FSCHG)
+MTREE_FSCHG=	-i
+.endif
+.endif

vangyzen added inline comments.Jul 10 2017, 6:01 PM

head/share/mk/bsd.init.mk
16–19 ↗	(On Diff #28919)	The reason was to make sure it was included as late as possible and therefore had final say on OWN/GRP. The reduced coverage didn't seem to be relevant, since I didn't see any problems in my testing. However, I don't remember testing `make distribution` or anything that descended into `etc`. Your patch seems fine to me. Sorry for the trouble.

vangyzen added inline comments.Jul 10 2017, 6:07 PM

head/share/mk/bsd.init.mk
16–19 ↗	(On Diff #28919)	We could move the logic to set _uid (and _gid) into a separate, possibly new file that could be included here and `etc/Makefile`. Do you think it's worth the effort, or should we wait until the next consumer pops up (if any)?