Paul -- Thanks for the suggestions. I took them as a starting point; it looks like much of distscript is moot these days, so I trimmed most of it out (it really only has to set repo_rev in VERSION these days).
Neat trick about perl -pie; I wasn't aware of that. FWIW: I like heredoc notation instead of a bunch of echo statements because it's just easier for the developer (e.g., if you need to reflow lines). I'm not worried about a minor performance penalty of writing to a temp file behind the scenes. Thanks! > On Apr 23, 2015, at 10:42 PM, Paul Hargrove <phhargr...@lbl.gov> wrote: > > I gave "make dist" a try on NetBSD (with its own /bin/sh) and Ubuntu Trusty > (w/ /bin/sh symlinked to dash). > Both generated the tarballs, but dash spewed some warnings on the unalias > commands. > > So here is my code review (roughly as long as the scipt itself). > > 1) > #!/usr/bin/env sh > > The presence of a bourne-compatible shell at /bin/sh is possible *the* most > fundamental assumption in any unix-like system. > So, the /bin/sh path is far more standard than /usr/bin/env (have seen > /bin/env). > Just use "#!/bin/sh" > > > 2) > > OMPI_VERSION=$3 > OMPI_REPO_REV=$4 > > if test x"$2" = x ; then > echo "Must supply relative distdir as argv[2] -- aborting" > exit 1 > elif test x$OMPI_VERSION = x ; then > echo "Must supply version as argv[1] -- aborting" > > The first line I've quoted takes OMPI_VERSION from ARGV[3]. > So the last line I quoted is incorrect in referring to "argv[1]" (should be > 3). > > 3) > > unalias cp &> /dev/null > unalias rm &> /dev/null > unalias mv &> /dev/null > > The redirect operator "&>" is a bash-ism. > With dash this runs unalias in the background (in a subshell) with only > stdout redirected. > The result is that if the alias exists it *remains*: > $ alias a=b > $ alias > a='b' > $ unalias a &> /dev/null > $ alias > a='b' > [1] + Done unalias a > > However, if the alias does not exist, dash will give you a warning since > stderr is not redirected: > $ unalias b &> /dev/null > $ unalias: b not found > [1] + Done(1) unalias b > > Neither of those is what you wanted. > The portable incantation for redirecting both stdout and stderr is > > command >/dev/null 2>&1 > > 4) > > ######################################################### > # VERY IMPORTANT: Now go into the new distribution tree # > ######################################################### > cd "$distdir" > > If it's important enough to yell like that, then I suggest checking that the > cd succeeded > > ######################################################### > # VERY IMPORTANT: Now go into the new distribution tree # > ######################################################### > cd "$distdir" > if test $? != 0; then > echo "*** ERROR: failed to enter $distdir" > exit 1 > fi > > 5) > > The current line > for file in $(echo $files) ; do > produces a behavior no different than the simpler > for file in $files; do > > HOWEVER, that is probably a moot point since the body of the loop appears to > be pointless. > I say pointless because it is intended to replace the string "OMPI_VERSION" > in README and INSTALL. > However, unless I am missing something, that string doesn't appear in either > file!! > > > 6) > > I am not sure why > > cat <<EOF > > line1 > line2 > line2 > > EOF > > is used rather than multiple echo commands. > The use of a "here document" normally requires the shell to write the text to > a temporary file to perform the input redirection. > > 7) > > In one place sed+cp+rm is used to edit a file. > In another sed+mv is used (but item #5 would eliminate that case). > Since perl is already required for autogen, you could replace sed+whatever > with perl's in-place operation > perl -pi -e 's/from/to/' -- file(s) > > > > -Paul > > > -- > Paul H. Hargrove phhargr...@lbl.gov > Computer Languages & Systems Software (CLaSS) Group > Computer Science Department Tel: +1-510-495-2352 > Lawrence Berkeley National Laboratory Fax: +1-510-486-6900 > _______________________________________________ > devel mailing list > de...@open-mpi.org > Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel > Link to this post: > http://www.open-mpi.org/community/lists/devel/2015/04/17354.php -- Jeff Squyres jsquy...@cisco.com For corporate legal information go to: http://www.cisco.com/web/about/doing_business/legal/cri/
