Re: [PATCH] newvers.sh

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.


> 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%/*}/..".


> -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, I don't believe you actually read the entire newvers.sh script.
(this is likely why you misread my patch in Message-ID:
<20100308010125.GA6387_at_dragon.NUXI.org>)  Did you get the proper output
in your testing?  From what I see, It causes a problem with the "${d}"
usage in this part of newvers.sh:

  #define VERSTR "${VERSION} #${v}${svn}${git}: ${t}\\n    ${u}_at_${h}:${d}\\n"

Thus when building with "make buildkernel", your patch produces vers.c as:

 #define VERSTR "FreeBSD 9.0-CURRENT #0 r204912M: Mon Mar 15 12:46:05 PDT 2010\n    rootk_at_dragon.NUXI.org:\n"

Instead of:

 #define VERSTR "FreeBSD 9.0-CURRENT #0 r204912M: Mon Mar 15 12:57:01 PDT 2010\n    rootk_at_dragon.NUXI.org:/usr/obj/MM/test/sys/GENERIC\n"


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


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%/*}/..
 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
 
 
_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
+"")
+	;;
+*)
 	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.

-- 
-- David  (obrien_at_FreeBSD.org)