From owner-freebsd-current@FreeBSD.ORG Tue Mar 16 00:41:24 2010 Return-Path: Delivered-To: freebsd-current@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 4A8EA106566B for ; Tue, 16 Mar 2010 00:41:24 +0000 (UTC) (envelope-from obrien@NUXI.org) Received: from dragon.nuxi.org (trang.nuxi.org [74.95.12.85]) by mx1.freebsd.org (Postfix) with ESMTP id 29ED08FC15 for ; Tue, 16 Mar 2010 00:41:23 +0000 (UTC) Received: from dragon.nuxi.org (obrien@localhost [127.0.0.1]) by dragon.nuxi.org (8.14.4/8.14.4) with ESMTP id o2G0fHqN037504; Mon, 15 Mar 2010 17:41:17 -0700 (PDT) (envelope-from obrien@dragon.nuxi.org) Received: (from obrien@localhost) by dragon.nuxi.org (8.14.4/8.14.4/Submit) id o2G0fH1g037503; Mon, 15 Mar 2010 17:41:17 -0700 (PDT) (envelope-from obrien) Date: Mon, 15 Mar 2010 17:41:17 -0700 From: "David O'Brien" To: "M. Warner Losh" Message-ID: <20100316004117.GB36963@dragon.NUXI.org> Mail-Followup-To: obrien@freebsd.org, "M. Warner Losh" , freebsd-current@FreeBSD.org References: <20100312171206.GA31761@dragon.NUXI.org> <20100313.211303.585238797224891349.imp@bsdimp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100313.211303.585238797224891349.imp@bsdimp.com> X-Operating-System: FreeBSD 9.0-CURRENT X-to-the-FBI-CIA-and-NSA: HI! HOW YA DOIN? User-Agent: Mutt/1.5.16 (2007-06-09) Cc: freebsd-current@FreeBSD.org Subject: Re: [PATCH] newvers.sh X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: obrien@FreeBSD.org List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Mar 2010 00:41:24 -0000 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) > @@ -44,7 +44,7 @@ > ${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@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}@${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@dragon.NUXI.org:\n" Instead of: #define VERSTR "FreeBSD 9.0-CURRENT #0 r204912M: Mon Mar 15 12:57:01 PDT 2010\n rootk@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) @@ -39,12 +39,13 @@ 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 @@ -87,27 +88,22 @@ 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` @@ -125,7 +121,7 @@ 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@FreeBSD.org)