This is the mail archive of the cygwin-apps mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: some comments on the generic build script


On Mon, 16 Jan 2006, Yitzchak Scott-Thoennes wrote:

> I'd like some feedback on changes I made to the gbs for
> fortune-1.99.1-2.

Sure.  FWICS, some of the changes below make sense as general improvements
to the g-b-s.

> --- gbs.sh      2006-01-15 17:46:43.875859200 -0800
> +++ fortune-1.99.1-2.sh 2006-01-15 22:26:48.129188800 -0800
> @@ -66,6 +66,8 @@
>  fi
>
>  export src_orig_pkg=${topdir}/${src_orig_pkg_name}
> +export debian_patch=fortune-mod_1.99.1-3.diff.gz
> +export debian_patch_name=${topdir}/$debian_patch

These variables seem to reverse the naming convention used in the rest of
the script.  See, for example, the definitions of src_pkg and
src_pkg_name...

>  # determine correct names for generated files
>  export src_pkg_name=${FULLPKG}-src.tar.bz2
> @@ -179,7 +184,8 @@
>  # change this if the original package was not tarred
>  # or if it doesn't unpack to a correct directory
>  unpack() {
> -  tar xv${opt_decomp}f "$1"
> +  tar xv${opt_decomp}f "$1" && \
> +  zcat $debian_patch_name | patch -p0
>  }

This makes sense, except that it should test for the presence of
$debian_patch (using conventional naming).  Also, I agree that
debian_patch could be renamed to upstream_patch.

>  mkdirs() {
> @@ -349,6 +352,7 @@
>    if [ -e ${src_orig_pkg}.sig ] ; then \
>      cp ${src_orig_pkg}.sig ${srcinstdir}/ ; \
>    fi && \
> +  cp ${debian_patch_name} ${srcinstdir}/${debian_patch} && \
>    cp $0 ${srcinstdir}/`basename $0` && \
>    name=$0 text="SCRIPT" sigfile && \
>    if [ "${SIG}" -eq 1 ] ; then \

Ditto on this hunk.

> @@ -195,7 +201,7 @@
>    unpack ${src_orig_pkg} && \
>    cd ${topdir} && \
>    if [ -f ${src_patch} ] ; then \
> -    patch -Z -p0 < ${src_patch} ;\
> +    patch -p0 < ${src_patch} ;\
>    fi && \
>    mkdirs && \
>    if [ -f ${topdir}/${log_pkg_name} ] ; then \
>
> These hunks include what are basically upstream patches and keep/regenerate
> my patches separately.
>
> The last hunk was needed because the debian patch file has no
> timestamps, so patch complained about the timestamp being not as
> expected for files in both it and my patch.

Umm, right.  However, I'd rather try keeping the timestamps if possible.
Is there a way to detect whether the patch has valid timestamps, or to
make patch ignore the timestamps if invalid?

> Would it be worthwhile doing s/debian_/upstream_/ and folding the
> other hunks in to the regular version, with the zcat|patch and cp
> omitted if the variable isn't set?

Yes, IMO.

> @@ -127,6 +129,9 @@
>         THANKS \
>         TODO \
>         USAGE \
> +       Offensive \
> +       cookie-files \
> +       debian \
>  "
>  export install_docs="`for i in ${install_docs}; do echo $i; done | sort -u`"
>  export test_rule=check
>
> These are added install_docs.

This looks like a patch that needs to be maintained locally.

> debian includes
> debian/{README.Debian,README.Debian.offensive,changelog,copyright}. At
> first, I had "debian/ \", because there seemed to be code to support
> that, but it included two copies of the files in the binary tarball,
> one under /usr/share/doc/fortune-1.99.1 and one under
> /usr/share/doc/fortune-1.99.1/debian.  This seemed a little odd.

Huh.  Probably a bug.  I'm surprised at the two copies, though -- the
current code will pass all the regular files directly to /usr/bin/install,
and, AFAIK, /usr/bin/install does not replicate directory structure...

There *is* a small bug in the directory handling code which the patch
below fixes (I'll check it in shortly), but it doesn't seem to be related
to this behavior.  Let me know if it fixes the problem for you, though.

------------------------------------------------------------------------------
Index: generic-build-script
===================================================================
RCS file: /cvs/cygwin-apps/packaging/templates/generic-build-script,v
retrieving revision 1.43
diff -u -p -r1.43 generic-build-script
--- generic-build-script        18 Oct 2005 05:01:36 -0000      1.43
+++ generic-build-script        17 Jan 2006 04:37:19 -0000
@@ -257,7 +257,7 @@ install() {
   templist="" && \
   for fp in ${install_docs} ; do \
     case "$fp" in \
-      */) templist="$templist `cd ${srcdir} && find $fp -type f`" ;;
+      */) templist="$templist `find ${srcdir}/$fp -type f`" ;;
       *)  for f in ${srcdir}/$fp ; do \
 	    if [ -f $f ] ; then \
 	      templist="$templist $f"; \
------------------------------------------------------------------------------

> Another minor issue was that install_docs has NOTES, which picked up
> the Notes file (which I wanted), but stored it with the name all in
> caps.

Interesting.  I usually have CYGWIN=check_case:strict, which would avoid
matching "Notes" with "NOTES".

> Is there an easy way in bash to go through a list of filenames and set
> the capitalization the way it actually is on disk?

I think setting "shopt -s nocaseglob" and changing "NOTES" to "[N]OTES"
should do it.

> @@ -203,16 +209,12 @@
>      tar xvjf ${topdir}/${log_pkg_name}
>    fi )
>  }
> +# fortune isn't autoconfusticated, just make objdir -> srcdir symlinks
>  conf() {
>    (cd ${objdir} && \
> -  CFLAGS="${MY_CFLAGS}" LDFLAGS="${MY_LDFLAGS}" \
> -  ${srcdir}/configure \
> -  --srcdir=${srcdir} --prefix="${prefix}" \
> -  --exec-prefix='${prefix}' --sysconfdir="${sysconfdir}" \
> -  --libdir='${prefix}/lib' --includedir='${prefix}/include' \
> -  --mandir='${prefix}/share/man' --infodir='${prefix}/share/info' \
> -  --libexecdir='${sbindir}' --localstatedir="${localstatedir}" \
> -  --datadir='${prefix}/share' 2>&1 | tee ${configurelogfile} )
> +  (cd ${srcdir} && find * -type d) | xargs mkdir -p && \
> +  (for dir in `find`; do find ${srcdir}/$dir -maxdepth 1 -type f | xargs ln -s -t $dir; done) && \
> +  touch ${configurelogfile} )
>  }
>  reconf() {
>    (cd ${topdir} && \
>
> No configure, so I just made symlinks under objdir for all the files
> under srcdir (except toplevel directories starting with .).

Did you happen to notice lnconf.sh, which does exactly what you have
above?

> Would it make sense to include something like that in the regular
> version whenever there's no configure file?

I've debated including the lnconf functionality directly into the g-b-s,
and figured that packages without "configure" could simply include
lnconf.sh (renamed to "configure").

> @@ -222,7 +224,7 @@
>  }
>  build() {
>    (cd ${objdir} && \
> -  make CFLAGS="${MY_CFLAGS}" 2>&1 | tee ${makelogfile} )
> +  make 2>&1 | tee ${makelogfile} )
>  }
>  check() {
>    (cd ${objdir} && \
>
> The Makefile set a lot of important stuff in CFLAGS= (including -O2)
> which this overrode.  Is this a common problem?  Should I have patched
> the Makefile to use a different variable name and include CFLAGS in it?

Good question.  Actually, perhaps we should instead ignore MY_CFLAGS if
it's empty...

> @@ -250,6 +252,7 @@
>      find ${instdir}${prefix}/share/info -name "*.info" | xargs -r gzip -q ; \
>    fi && \
>    if [ -d ${instdir}${prefix}/share/man ] ; then \
> +    true "breaks cause unstr.8 is symlink to strfile.8" || \
>      find ${instdir}${prefix}/share/man -name "*.1" -o -name "*.3" -o \
>        -name "*.3x" -o -name "*.3pm" -o -name "*.5" -o -name "*.6" -o \
>        -name "*.7" -o -name "*.8" | xargs -r gzip -q ; \
>
> Took me a while to figure out why this was breaking things.  I have
> these:
>
> -rw-r--r-- 1 sthoenna None 11485 Jan 15 22:27 ./man6/fortune.6
> -rw-r--r-- 1 sthoenna None  7512 Jan 15 22:27 ./man8/strfile.8
> lrwxrwxrwx 1 sthoenna None     9 Jan 15 22:27 ./man8/unstr.8 -> strfile.8
>
> and when gzip tried to compress unstr.8, strfile.8 was no longer there
> (having become strfile.8.gz), causing the whole build process to stop.
> Any easy fixes to suggest?

Hmm, interesting.  Perhaps add a "-type f" to the "find" command, and then
have another "find" that recreates symlinks with the .gz extension
pointing to the gzipped versions of files?

> @@ -331,7 +334,7 @@
>    unpack ${src_orig_pkg} && \
>    mv ${BASEPKG} ../${BASEPKG}-orig && \
>    cd ${topdir} && \
> -  diff -urN -x '.build' -x '.inst' -x '.sinst' \
> +  diff -p0rN4u -x '.build' -x '.inst' -x '.sinst' -x '.buildlogs' \
>      ${BASEPKG}-orig ${BASEPKG} > \
>      ${srcinstdir}/${src_patch_name} ; \
>    rm -rf ${BASEPKG}-orig )
>
> Think of the Children.

Aside from providing a nice mnemonic, what do the -0 and -4 do?  Also,
traditionally the diff did not include function names -- I'm not opposed
to changing that, though...

> And .buildlogs was missing.

Yep, thanks.

> Thanks for any comments.

If you could clean up some of the above and make it more general, a lot of
it would be useful in the g-b-s, IMO.  Thanks for sharing this.

BTW, a heads-up -- I'm working on a way to keep the logging without
breaking the "stop-on-error" behavior (haven't touched the code in a
while, but now got back to it).  The changes will be rather extensive, but
go ahead and work off the current CVS -- I should be able to merge your
patch in when the time comes.
	Igor
-- 
				http://cs.nyu.edu/~pechtcha/
      |\      _,,,---,,_	    pechtcha@cs.nyu.edu | igor@watson.ibm.com
ZZZzz /,`.-'`'    -.  ;-;;,_		Igor Peshansky, Ph.D. (name changed!)
     |,4-  ) )-,_. ,\ (  `'-'		old name: Igor Pechtchanski
    '---''(_/--'  `-'\_) fL	a.k.a JaguaR-R-R-r-r-r-.-.-.  Meow!

"Las! je suis sot... -Mais non, tu ne l'es pas, puisque tu t'en rends compte."
"But no -- you are no fool; you call yourself a fool, there's proof enough in
that!" -- Rostand, "Cyrano de Bergerac"


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]