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

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

[ira.weiny]

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 create the rpmbuild directory if the user does not already
have it.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 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..26e29bd7953d 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
+	mkdir -p $OUTDIR
+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"
 
-- 
2.20.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] ndctl: Generalized make-git-snapshot.sh

[Dan Williams]

On Fri, Feb 15, 2019 at 11:21 AM <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 create the rpmbuild directory if the user does not already
> have it.
>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  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..26e29bd7953d 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
> +       mkdir -p $OUTDIR

Shouldn't this be rpmdev-setuptree?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] ndctl: Generalized make-git-snapshot.sh

[Ira Weiny]

On Fri, Feb 15, 2019 at 11:31:46AM -0800, Dan Williams wrote:
> On Fri, Feb 15, 2019 at 11:21 AM <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 create the rpmbuild directory if the user does not already
> > have it.
> >
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  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..26e29bd7953d 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
> > +       mkdir -p $OUTDIR
> 
> Shouldn't this be rpmdev-setuptree?

Sure looks like it...

I never knew about that command...

Ira

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[ira.weiny]

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`
+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"
 
-- 
2.20.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Verma, Vishal L]

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

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Ira Weiny]

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"
> >  
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Verma, Vishal L]

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

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH V3] ndctl: Generalized make-git-snapshot.sh

[ira.weiny]

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 error out if not run in the root of the git tree
and create the output directory if the user does not already have it.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes since V2
	Don't use $0 to determine git tree location
	Instead expect user to run within root of git tree
		Error out if not

Changes since V1
	use rpmdev-setuptree

 make-git-snapshot.sh | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/make-git-snapshot.sh b/make-git-snapshot.sh
index 142419d623fe..0b5de17a6d5c 100755
--- a/make-git-snapshot.sh
+++ b/make-git-snapshot.sh
@@ -2,10 +2,21 @@
 set -e
 
 NAME=ndctl
-REFDIR="$HOME/git/ndctl"  # for faster cloning, if available
+
+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
 
+if [ ! -d $OUTDIR ]; then
+	mkdir -p $OUTDIR
+fi
+
 [ -n "$1" ] && HEAD="$1" || HEAD="HEAD"
 
 WORKDIR="$(mktemp -d --tmpdir "$NAME.XXXXXXXXXX")"
-- 
2.20.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm