On Wed, Nov 05, 2008 at 01:40:39AM -0600, Norm Jacobs wrote:
>
> You made a statement about your tarball in your mail that is
> disturbing. Your tarball should be the exact tarball that you pulled
> down from the component's ftp or web site. If you need to apply patches
> to the unpacked source, that's "ok", though we want you to file bugs for
> things you are fixing with patches and submit them upstream. Hopefully,
> as the component evolves, it will incorporate your changes and require
> less (preferably no) patching in a future version. Specific comments
> are below.
I did a cvs checkout and tar'd it up. The quilt cvs repo represents version
0.47 plus a few fixes that have gone in over the past few weeks; some of those
fixes are a result of this porting effort. Two of the patches from my first
webrev were accepted over the weekend, so for the second webrev I did another
checkout and made another tarball.
For those CRs that I open for the patches, when should those CRs be closed?
Do open CRs prevent me from doing a putback into SFW?
> usr/src/cmd/quilt/Makefile.sfw
>
> There is a mechanism in place that will automatically unpack your
> tarball and apply all patches in
> .../src/quilt/Patches/[0-9][0-9]-*.patch if you have an appropriate
> dependency ($(VER)/.patched) in your Makefile. See
> usr/src/cmd/{cups|ghostscript|gimp-print|hplip|top|zsh}
I'll poke at it, see how it goes.
> Are the various --with-{util} options build or runtime dependencies?
Would you elaborate?
> Line 73: Why are the CFLAGS different than those that you use to
> actually build the bits?
I should remove that line. The configure script handles this type of thing.
> Line 91: Use the $(RM) macro in place of rm "-$(RM) -r $(APP)"
Done.
> usr/src/cmd/quilt/install-sfw
>
> Lines 34-35 define an unused variable.
The comments should mention only those variables that I actually use?
> You might consider using the component's "install" target to
> populate the proto area.
I see, $(ROOT) is the proto area.
I'm running into a bit of a mess because quilt's configure script is getting
'/opt/onbld/bin/i386/install -c' for the AC_PROG_INSTALL macro, and -c is an
invalid option. It seems to me that autoconf in SFW should have gotten this
right?
Given this complication, I'd rather not abandon the install-sfw approach.
> usr/src/cmd/quilt/quilt-0.47.tar.gz
>
> This is missing from your webrev and as a result, I am assuming that
> it's not under teamware control. You will need to use "wx create"
> to add it to your workspace or you will end up breaking the gate if
> your RTI advocate doesn't catch it.
I wasn't sure about having binary files in webrev.
I'll add it.
> usr/src/cmd/quilt/patch.find-path
>
> It would seem to me that you should be generating this patch like so
> that it functions like the other --with-{util} configure options and
> try to submit it upstream.
The comment at the top of the patch indicates this patch can go away after GNU
find (LSARC/2008/531) is integrated into SFW. I guess I would rather take
that route.
> usr/src/cmd/quilt/patch.guards-man
>
> You should use pod2man to generate the man page from the perl file
> during the build instead of patching this file in and having to
> updated the patch with each update. If you were to use the
> "install" target from the component to populate the proto area, this
> would happen automatically, but it appears that the component
> Makefiles may need work to honor DESTDIR.
The regular quilt 'make install' step does not attempt to run pod2man, or
to install the guards manpage.
Today this patch creates quilt-0.47/doc/guards.1. Instead of that, I could
modify the 'install' target in Makefile.sfw to run:
(cd $(APP); \
/usr/perl5/bin/pod2man bin/guards.in > doc/guards.1)
> usr/src/cmd/quilt/patch.import-zcat
>
> This patch should be submitted back upstream. They are clearly
> unpacking a gzipped file using zcat(1), where gzcat(1) is the
> correct command to use.
No, on linux machines 'zcat' is GNU zcat. And they don't have or need a thing
called gzcat.
The quilt test suite is not designed with .in files, so I'm a bit restricted
in what I can do for this problem without turning it into a much larger
production.
> usr/src/cmd/quilt/patch.no-bash_completion
>
> This patch is unnecessary. You appear to be installing using an
> install-sfw that is copying over bits from your build tree into your
> proto area. If you had chosing to use the components "install"
> target (cd $(VER) ; env - ... make ... install), you could have
> removed the bits that you don't want from the proto area. Either
> way, you don't need this patch and it's one less thing to regenerate
> when you update this software.
I'll remove this patch.
> usr/src/cmd/quilt/patch.refresh-z
>
> presumably this won't go back upstream, but won't be needed if/when
> we ship GNU getopts
Yes, as noted in the comments at the top of the patch.
> usr/src/cmd/quilt/patch.refresh-z-test
>
> hopefully you can file a bug and send this upstream
No, this problem will go away when solaris has GNU getopts, as noted at the
top of the patch.
> usr/src/cmd/quilt/patch.with-gtar
>
> hopefully you can file a bug and send this upstream
Yes, I agree.
> usr/src/cmd/quilt/patch.with-xgettext
>
> Is this something that you can send upstream?
Yes, but not this particular version.
> usr/src/cmd/quilt/patch.without-patch-wrapper
>
> Will this be accepted upstream or will a future revision no longer
> need it?
I think this patch, or something close to it, will be accepted.
> usr/src/pkgdefs/SUNWquiltr/prototype_com
>
> Line 49: is this a config file that you want to preserve on
> upgrade? That they user will edit? Should the type be 'e' instead
> of 'f'? Should the class be 'preserve'? Should the file be writable?
I've changed it to 'e', because I do think an administrator might want to
customize it. I've changed the class to 'preserve'. I think 0644 seems
correct.
Thanks, Norm. I'll generate another webrev later today.
Dean
