On Fri, 2019-02-15 at 13:47 -0800, Ira Weiny wrote:
> On Fri, Feb 15, 2019 at 01:01:27PM -0800, 'Vishal Verma' wrote:
> > On Fri, 2019-02-15 at 12:06 -0800, [email protected] wrote:
> > > From: Ira Weiny <[email protected]>
> > >
> > > make-git-snapshot.sh made an assumption of the git tree location.
> > >
> > > Furthermore, it assumed the user has an rpmbuild environment directory
> > > structure set up.
> > >
> > > Enhance the script to figure out where in what location it has been
> > > cloned and setup the rpmbuild tree if the user does not already
> > > have it.
> > >
> > > Signed-off-by: Ira Weiny <[email protected]>
> > >
> > > ---
> > > Changes since V1
> > > use rpmdev-setuptree
> > >
> > > make-git-snapshot.sh | 12 ++++++++++--
> > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/make-git-snapshot.sh b/make-git-snapshot.sh
> > > index 142419d623fe..513086b1f93d 100755
> > > --- a/make-git-snapshot.sh
> > > +++ b/make-git-snapshot.sh
> > > @@ -2,10 +2,18 @@
> > > set -e
> > >
> > > NAME=ndctl
> > > -REFDIR="$HOME/git/ndctl" # for faster cloning, if available
> > > +
> > > +pushd `dirname $0`
> > > +REFDIR=`pwd`
> >
> > We should use the $() syntax for command substitution.
> > Also quote it - pushd "$(dirname "$0")"
> > And for setting REFDIR, no need to execute the pwd command - just use
> > the $PWD variable from the environment.
>
> Probably irrelevant with your comments below.
>
> > Additionally, $0 is not a reliable way to get the location of the
> > script. It is probably best to make an assumption that the script will
> > be run from the top level of an ndctl tree, and test for that instead of
> > trying to deduce where the script is being run from and trying to cd to
> > it.
>
> When can $0 not be the name of the script?
For the common case of calling ./script, it is probably fine. But
consider for example:
$ bash < /tmp/test
script was called as bash
Or say if someone decided to symlink it somewhere for convenience:
$ readlink -f /usr/bin/test-symlink && test-symlink
/tmp/test
script was called as /usr/bin/test-symlink
Agreed that in this case those are unlikely, but the point is using $0
for 'locating yourself' is just fragile and should be avoided.
See this for more commentary:
http://mywiki.wooledge.org/BashFAQ/028
>
> > We only really need the git-version script, and if we can test for its
> > presence, then we can relatively safely assume we're in the right
> > location. If it is not a git tree then we can leave it for git-archive
> > to complain. So something like:
> >
> > test -x "./git-version"
>
> Ah ok... Then the stuff above is irrelevant.
>
> And I would prefer something more explicit than just returning.
>
> But the real question is if this is really supposed to be run by a user or
> not.
> If not then your way is better.
>
> diff --git a/make-git-snapshot.sh b/make-git-snapshot.sh
> index 513086b1f93d..fccf1da0d61b 100755
> --- a/make-git-snapshot.sh
> +++ b/make-git-snapshot.sh
> @@ -3,9 +3,12 @@ set -e
>
> NAME=ndctl
>
> -pushd `dirname $0`
> -REFDIR=`pwd`
> -popd
> +if [ ! -x ./git-version ]; then
> + echo "$0 : ERROR: Must run from top level of git tree"
> + exit 1
> +fi
> +
> +REFDIR=$PWD
Yes this looks fine to me.
>
> UPSTREAM=$REFDIR #TODO update once we have a public upstream
> OUTDIR=$HOME/rpmbuild/SOURCES
>
>
> > should suffice.
> >
> > > +popd
> > > +
> > > UPSTREAM=$REFDIR #TODO update once we have a public upstream
> > > OUTDIR=$HOME/rpmbuild/SOURCES
> > >
> > > +if [ ! -d $OUTDIR ]; then
> > > + rpmdev-setuptree
> > > +fi
> > > +
> >
> > There was some discussion around this a while back: [1] and [2]
> >
> > But we had decided that rpmdev-setuptree was heavy handed in that it can
> > overwrite user configuration and we shouldn't use it. Instead in [2], we
> > added a test for the rpmdev tree to exist, and bail if it doesn't.
> >
> > The one improvement I can see to this is:
> >
> > if [ ! -d $OUTDIR ]; then
> > mkdir -p $OUTDIR
> > fi
>
> Dan?
>
> Ira
>
> > That way if components of OUTDIR are missing, we can make those
> > directories only, which is harmless.
> >
> > [1]: https://lists.01.org/pipermail/linux-nvdimm/2017-November/013378.html
> > [2]: https://lists.01.org/pipermail/linux-nvdimm/2017-November/013408.html
> >
> > > [ -n "$1" ] && HEAD="$1" || HEAD="HEAD"
> > >
> > > WORKDIR="$(mktemp -d --tmpdir "$NAME.XXXXXXXXXX")"
> > > @@ -14,7 +22,7 @@ trap 'rm -rf $WORKDIR' exit
> > > [ -d "$REFDIR" ] && REFERENCE="--reference $REFDIR"
> > > git clone $REFERENCE "$UPSTREAM" "$WORKDIR"
> > >
> > > -VERSION=$(./git-version)
> > > +VERSION=$($REFDIR/git-version)
> > > DIRNAME="ndctl-${VERSION}"
> > > git archive --remote="$WORKDIR" --format=tar --prefix="$DIRNAME/" HEAD |
> > > gzip > $OUTDIR/"ndctl-${VERSION}.tar.gz"
> > >
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
