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

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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	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

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	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	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)?