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