Re: [PATCH] newvers.sh

David

In message: <20100316004117.GB36963_at_dragon.NUXI.org>
            "David O'Brien" <obrien_at_freebsd.org> writes:
: On Sat, Mar 13, 2010 at 09:13:03PM -0700, M. Warner Losh wrote:
: > The Makefile already knows where the kernel src is located.  Let's use
: > that knowledge to make things a little simpler.  This also uses the
: > Makefile variable SYSDIR.  It should also work with non-standard sys
: > directories.
: ..
: > Index: conf/kern.post.mk
: >  vers.c: $S/conf/newvers.sh $S/sys/param.h ${SYSTEM_DEP}
: > -	MAKE=${MAKE} sh $S/conf/newvers.sh ${KERN_IDENT}
: > +	MAKE=${MAKE} SYSDIR=$S sh $S/conf/newvers.sh ${KERN_IDENT}
: 
: I'd rather not introduce yet more special things that have to be done
: before invoking newvers.sh.   ("MAKE=${MAKE} sh" is not an issue as the
: script works if MAKE is not passed in given it has "${MAKE:-make}").
: 
: The script can be more self contained than this, and I think that is
: technically better.

OK.  I think that passing the info in isn't that big a deal and makes
the dependency more explicit.

: > Index: conf/newvers.sh
: > ===================================================================
: > --- conf/newvers.sh	(revision 204938)
: > +++ conf/newvers.sh	(working copy)
: > _at__at_ -44,7 +44,7 _at__at_
: >  		${PARAMFILE})
: >  else
: >  	RELDATE=$(awk '/__FreeBSD_version.*propagated to newvers/ {print $3}' \
: > -		$(dirname $0)/../sys/param.h)
: > +		${SYSDIR}/sys/param.h)
: 
: I don't think we should depend on having SYSDIR defined before invoking
: newvers.sh.  Its worse than requiring that as a parameter.  We don't set
: KERN_IDENT=$KERN_IDENT before invoking newvers.sh.
: 
: Either
:     MAKE=${MAKE} sh $S/conf/newvers.sh ${KERN_IDENT} $S
: or
:     MAKE=${MAKE} SYSDIR=$S KERN_IDENT=$KERN_IDENT sh $S/conf/newvers.sh
: 
: for regularity.  But I really feel we can trust 'newvers.sh' to be within
: the kernel sources directory - thus "$(dirname $0)/.." is a
: self-contained method to determining what the kernel directory is.
: No guessing.  This can be optimized to "${0%/*}/..".

You are trading one dependency for another here.  Either you pass it
in by typing the path to newvers.sh, or you pass it in directly from
the kernel build.  But it would be cleaner if it were passed in as the
first parameter.

: > -v=`cat version` u=${USER:-root} d=`pwd` h=${HOSTNAME:-`hostname`} t=`date`
: > +v=`cat version` u=${USER:-root} h=${HOSTNAME:-`hostname`} t=`date`
: 
: Unfortunately, [snipped] [[ ${d} is used later, and silent succeeds
: with the wrong results with your patch ]]

$d is used later.  Gotcha.  I missed that.

: > -case "$d" in
: > -*/sys/*)
: ..
: > +for dir in /bin /usr/bin /usr/local/bin; do
: > +	if [ -d "${SYSDIR}/.svn" -a -x "${dir}/svnversion" ] ; then
: > +		svnversion=${dir}/svnversion
: ..
: > -	for dir in /bin /usr/bin /usr/local/bin; do
: > -		if [ -d "${SRCDIR}/sys/.svn" -a -x "${dir}/svnversion" ] ; then
: 
: Are you implicitly depending on there not being a '.svn/' in the root
: directory?

Assuming that you violated the precondition of passing SYSDIR in, then
yes.  However, I don't think that's a big deal.

: The invocation of 'newvers.sh' elsewhere in the tree will not
: have 'SYSDIR' (of your patch) set, so the test will be (last iteration):
: 
:     if [ -d "/.svn" -a -x "$/usr/local/bin/svnversion" ] ; then
: 
: I'd rather not limit the user to not having '/.svn' that is used to
: track configuration files, etc...
:
: This patch is the end version I was working to (thru a series of
: changes):
: 
: * Simplify SRCDIR calculation by directly finding the kernel sources
:   based directly on one of them.
: * Rename SRCDIR to KERN_TOPDIR to be more clear which sources these are,
:   and at what level
: * git isn't in the base system and being GPL'ed, likely never will.
: * Revisit r196435 - rather than guess if 'newvers.sh' is being
:   invoked as part of the kernel build or not based on a path (proven
:   to be fragile), key off of having a KERN_IDENT.

Comments below...

: Index: newvers.sh
: ===================================================================
: --- newvers.sh	(revision 204939)
: +++ newvers.sh	(working copy)
: _at__at_ -39,12 +39,13 _at__at_ fi
:  RELEASE="${REVISION}-${BRANCH}"
:  VERSION="${TYPE} ${RELEASE}"
:  
: +KERN_TOPDIR=${0%/*}/..

I'd have selected SYSDIR, which is used elsewhere in the build system
to indicate the top of the kernel tree.

:  if [ "X${PARAMFILE}" != "X" ]; then
:  	RELDATE=$(awk '/__FreeBSD_version.*propagated to newvers/ {print $3}' \
:  		${PARAMFILE})
:  else
:  	RELDATE=$(awk '/__FreeBSD_version.*propagated to newvers/ {print $3}' \
: -		$(dirname $0)/../sys/param.h)
: +		${KERN_TOPDIR}/sys/param.h)
:  fi
:  

If we move this "grep" into include/Makefile, then we can eliminate
this entire conditional, leaving only the else part of it.

: _at__at_ -87,27 +88,22 _at__at_ touch version
:  v=`cat version` u=${USER:-root} d=`pwd` h=${HOSTNAME:-`hostname`} t=`date`
:  i=`${MAKE:-make} -V KERN_IDENT`
:  
: -case "$d" in
: -*/sys/*)
: -	SRCDIR=${d##*obj}
: -	if [ -n "$MACHINE" ]; then
: -		SRCDIR=${SRCDIR##/$MACHINE}
: -	fi
: -	SRCDIR=${SRCDIR%%/sys/*}
: -
: +case "$i" in
: +"")
: +	;;

This is only needed because newvers.sh is invoked from
src/include/Makefile.  If that were elimianted, the case could also be
eliminated as well, since there'd be no other users of newvers.sh.

: +*)
:  	for dir in /bin /usr/bin /usr/local/bin; do
: -		if [ -d "${SRCDIR}/sys/.svn" -a -x "${dir}/svnversion" ] ; then
: +		if [ -d "${KERN_TOPDIR}/.svn" -a -x "${dir}/svnversion" ] ; then
:  			svnversion=${dir}/svnversion
:  			break
:  		fi
: -		if [ -d "${SRCDIR}/.git" -a -x "${dir}/git" ] ; then
: -			git_cmd="${dir}/git --git-dir=${SRCDIR}/.git"
: -			break
: -		fi
:  	done
: -
:  	if [ -n "$svnversion" ] ; then
: -		svn=" r`cd ${SRCDIR}/sys && $svnversion`"
: +		svn=" r`cd ${KERN_TOPDIR} && $svnversion`"
: +	fi
: +
: +	if [ -z "$svnversion" -a -d "${KERN_TOPDIR}/../.git" -a -x /usr/local/bin/git ] ; then
: +		git_cmd="/usr/local/bin/git --git-dir=${SRCDIR}/.git"
:  	fi
:  	if [ -n "$git_cmd" ] ; then
:  		git=`$git_cmd rev-parse --verify --short HEAD 2>/dev/null`
: _at__at_ -125,7 +121,7 _at__at_ case "$d" in
:  				git=" ${git}"
:  			fi
:  		fi
: -		if $git_cmd --work-tree=${SRCDIR} diff-index \
: +		if $git_cmd --work-tree=${KERN_TOPDIR}/.. diff-index \
:  		    --name-only HEAD | read dummy; then
:  			git="${git}-dirty"
:  		fi
:
: We can simplify this farther by looking at the other use of 'newvers.sh'
: and realizing that:
: 
:         RELDATE=$(awk '/__FreeBSD_version.*propagated to newvers/ {print $3}' \
: 	                ${PARAMFILE})
: 
: is just a fancy grep of param.h... And that it would much more direct to
: do the grepping in /usr/src/include/Makefile than using newver.sh to do
: affective the same thing.

Having looked at things, I don't know why we need to invoke newvers.sh
as part of generating osreldate.h in the first place.  It leaves
vers.c and version around as extra clean files that need to be removed
later.  Seems to be that expanding RELDATE in include/Makefile would
be a reasonable thing to do, or better yet, just grepping the lines
out of sys/param.h: we already depend in it being in a specific format
so this wouldn't be a new coupling.  We wouldn't even need to invoke
the kernel build system to get this information, and would eliminate
the need to do the case statement above.  It seems like we're going to
a lot of effort to reuse 1 line of newvers.sh which far exceeds that
one line savings.  I'm also unsure that this automatically generated
file really has copyright protection, so including a copyright notice
isn't necessary. The project would lose nothing by explicitly
declaring it to be in the public domain.

osreldate.h: ${.CURDIR}/../sys/conf/newvers.sh ${.CURDIR}/../sys/sys/param.h \
    ${.CURDIR}/Makefile
	_at_${ECHO} creating osreldate.h from newvers.sh
	_at_MAKE=${MAKE}; \
	PARAMFILE=${.CURDIR}/../sys/sys/param.h; \
	. ${.CURDIR}/../sys/conf/newvers.sh; \
	echo "$$COPYRIGHT" > osreldate.h; \
	echo "#ifdef _KERNEL" >> osreldate.h; \
	echo "#error \"<osreldate.h> cannot be used in the kernel, use <sys/param.h>\"" >> osreldate.h; \
	echo "#else" >> osreldate.h; \
	echo "#undef __FreeBSD_version" >> osreldate.h; \
	echo "#define __FreeBSD_version $$RELDATE" >> osreldate.h; \
	echo "#endif" >> osreldate.h

would become:

osreldate.h: ${.CURDIR}/../sys/sys/param.h ${.CURDIR}/Makefile
	_at_${ECHO} creating osreldate.h from param.h
	echo "/* This file is in the public domain */" > osreldate.h; \
	echo "#ifdef _KERNEL" >> osreldate.h; \
	echo "#error \"<osreldate.h> cannot be used in the kernel, use <sys/param.h>\"" >> osreldate.h; \
	echo "#else" >> osreldate.h; \
	grep "^#.*__FreeBSD_version" ${.CURDIR}/../sys/sys/param.h >> osreldate.h; \
	echo "#endif" >> osreldate.h

so it would be simpler in multiple places.

Warner