diff mbox series

[V2] ndctl: Generalized make-git-snapshot.sh

Message ID 20190215200614.30004-1-ira.weiny@intel.com (mailing list archive)
State Superseded
Headers show
Series [V2] ndctl: Generalized make-git-snapshot.sh | expand

Commit Message

Ira Weiny Feb. 15, 2019, 8:06 p.m. UTC
From: Ira Weiny <ira.weiny@intel.com>

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 <ira.weiny@intel.com>

---
Changes since V1
	use rpmdev-setuptree

 make-git-snapshot.sh | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Vishal Verma Feb. 15, 2019, 9:01 p.m. UTC | #1
On Fri, 2019-02-15 at 12:06 -0800, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> 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 <ira.weiny@intel.com>
> 
> ---
> 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.

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.

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"

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

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"
>
Ira Weiny Feb. 15, 2019, 9:47 p.m. UTC | #2
On Fri, Feb 15, 2019 at 01:01:27PM -0800, 'Vishal Verma' wrote:
> 
> On Fri, 2019-02-15 at 12:06 -0800, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > 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 <ira.weiny@intel.com>
> > 
> > ---
> > 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?

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

 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"
> >  
>
Vishal Verma Feb. 15, 2019, 10:41 p.m. UTC | #3
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, ira.weiny@intel.com wrote:
> > > From: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > 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 <ira.weiny@intel.com>
> > > 
> > > ---
> > > 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"
> > >
diff mbox series

Patch

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`
+popd
+
 UPSTREAM=$REFDIR #TODO update once we have a public upstream
 OUTDIR=$HOME/rpmbuild/SOURCES
 
+if [ ! -d $OUTDIR ]; then
+	rpmdev-setuptree
+fi
+
 [ -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"