Expanding on NO_ROOT: Categorizing installed files

[Sean Fagan]

On Jul 10, 2014, at 8:35 AM, Brooks Davis <brooks at freebsd.org> wrote:

> I very much like the functionalty and think it's a good idea.

It was mostly Xin and Jordan's idea :).  ("Wouldn't it be nice if we could
use that somehow?  Sean, go do that.")

>  I don't
> understand why you didn't use the existing -T/tags= mechanism in
> install which we're already using for debug info.

A couple of reasons:  first, because it wasn't clear to me who was using -T,
and I didn't want to alter the functionality of that; second, because I wasn't clear
how one would have multiple tags.  (The latter is not a huge deal, I can obviously
code that up easily enough, and make a comma-separated list in install.)

I had initially called it "package", btw, and that shows up in some places still.
"category" seemed like a better name.

> 
>> diff --git a/Makefile.inc1 b/Makefile.inc1
>> index c0591b6..b9edd0d 100644
>> --- a/Makefile.inc1
>> +++ b/Makefile.inc1
>> @@ -14,6 +14,7 @@
>> #	-DNO_KERNELOBJ do not run ${MAKE} obj in ${MAKE} buildkernel
>> #	-DNO_PORTSUPDATE do not update ports in ${MAKE} update
>> #	-DNO_ROOT install without using root privilege
>> +#	-DLOG_META_INFO Log metadata about installed files
> 
> I don't see much value in supporting the metadata log in the install as
> root case.  Is there are reason it's needed?

I initially only used NO_ROOT.  However, then when I started making packages
using that, I found out that I really did need to have the install run as root, because
I could not specify all of the mode bits in the PKGNG manifest, and thus I needed
tar to pick them up.  (Eventually, admittedly, I did end up using the python tarfile
module, and so that's an option.  However, getting the right bits in the tarfile module
in python is a pain, and it's easier to let it set things from the actual filesystem.)
Further, I didn't assume everyone who wanted to use this would want to do full packaging,
but might want to instead simply use grep and tar to pick which files.  Or many other
things.

So I opted for the most flexible variant, which was to keep NO_ROOT behaving as it did,
and add another case that used most of the work from NO_ROOT.

Does that make sense?

>> 
>> distributekernel distributekernel.debug:
>> diff --git a/bin/Makefile b/bin/Makefile
>> index e5052ca..ca218ac 100644
>> --- a/bin/Makefile
>> +++ b/bin/Makefile
>> @@ -1,6 +1,9 @@
>> #	From: @(#)Makefile	8.1 (Berkeley) 5/31/93
>> # $FreeBSD$
>> 
>> +META_CATEGORY= base
> 
> I belive this should be in bin/Makefile.inc which will eliminate the need
> for .EXPORTVAR.

I will try that later; thanks.

>> diff --git a/lib/csu/amd64/Makefile b/lib/csu/amd64/Makefile
>> index afe7fe6..5616e04 100644
>> --- a/lib/csu/amd64/Makefile
>> +++ b/lib/csu/amd64/Makefile
>> @@ -9,6 +9,9 @@ CFLAGS+=	-I${.CURDIR}/../common \
>> 		-I${.CURDIR}/../../libc/include
>> CFLAGS+=	-fno-omit-frame-pointer
>> 
>> +META_CATEGORY=	base
>> +.EXPORTVAR: META_CATEGORY
>> +
> 
> I think the .EXPORTVAR is gratutious here and in the other
> lib/*/Makefiles.  For that matter, I don't understand why it's needed at
> all given the presence of META_CATEGORY in lib/Makefile.

I didn't put it in all of the Makefiles, and the reason for having it in
the environment was to allow for sub-directories to pick it up; it also
allows for them to easily over-ride it.  (E.g., for any library in src/lib
which someone might decide shouldn't really be part of base.)

>> diff --git a/share/man/man9/Makefile b/share/man/man9/Makefile
>> index dfa450e8..268ce8a 100644
>> --- a/share/man/man9/Makefile
>> +++ b/share/man/man9/Makefile
>> @@ -1,5 +1,8 @@
>> # $FreeBSD$
>> 
>> +META_CATEGORY=	kernel
> 
> I can see some loging in this, but it seems like a somewhat odd choice.

Odd choice how?

>> 
>> diff --git a/share/mk/bsd.links.mk b/share/mk/bsd.links.mk
>> index 1e4d57e..c9f83f0 100644
>> --- a/share/mk/bsd.links.mk
>> +++ b/share/mk/bsd.links.mk
>> @@ -8,7 +8,8 @@ afterinstall: _installlinks
>> .ORDER: realinstall _installlinks
>> _installlinks:
>> .if defined(LINKS) && !empty(LINKS)
>> -	@set ${LINKS}; \
>> +	@echo LINKFOO
>> +	set ${LINKS}; \
> 
> This looks like a debug leftover.
> 
>> 	while test $$# -ge 2; do \
>> 		l=${DESTDIR}$$1; \
>> 		shift; \
>> @@ -19,7 +20,8 @@ _installlinks:
>> 	done; true
>> .endif
>> .if defined(SYMLINKS) && !empty(SYMLINKS)
>> -	@set ${SYMLINKS}; \
>> +	@echo SYMFOO
>> +	set ${SYMLINKS}; \
> 
> This too.

snicker.  Yeah, sorry.  I thought I'd gotten rid of those after I figured out what
to do :).

>> diff --git a/sys/conf/kmod.mk b/sys/conf/kmod.mk
>> index cd11e3a..ea50d33 100644
>> --- a/sys/conf/kmod.mk
>> +++ b/sys/conf/kmod.mk
>> @@ -68,6 +68,10 @@ KMODLOAD?=	/sbin/kldload
>> KMODUNLOAD?=	/sbin/kldunload
>> OBJCOPY?=	objcopy
>> 
>> +.if defined(META_CATEGORY)
>> +META_LOG_SYMBOLS=	-P ${META_CATEGORY}:dev
> 
> There seem to be more spellings of META_LOG_SYMBOLS than necessicary
> (_META_INFO).

I'm not sure what you mean?

Thank you very much for the feedback!

(Replying to just the list for now; if you would rather I reply to you as well in the future, please let me know.)

Sean.