Date: Mon, 15 Mar 2010 17:41:17 -0700 From: "David O'Brien" <obrien@FreeBSD.org> To: "M. Warner Losh" <imp@bsdimp.com> Cc: freebsd-current@FreeBSD.org Subject: Re: [PATCH] newvers.sh Message-ID: <20100316004117.GB36963@dragon.NUXI.org> In-Reply-To: <20100313.211303.585238797224891349.imp@bsdimp.com> References: <20100312171206.GA31761@dragon.NUXI.org> <20100313.211303.585238797224891349.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100316004117.GB36963>