Jason Li wrote: > Hi Jim and team, > > I have passed the i-team code review & ARC review already. > > Before going onto the c-team review step, I did a full functional testing > for SUNWpatchutils, and found some issues(bug/dependency) > > 1. Most utilities in this package handle patch files with gpatch & gdiff, > so we need to add two more packages into the depend file, as follows: > > ... > P SUNWgnu-diffutils GNU diffutils > P SUNWgpch The GNU Patch utility
Ok. I will make the minor update to the ARC case once you get closer to putback. > 2. I found a bug that the getline() function in the file src/util.c > provided for non-GNU systems can't handle NUL character properly. > > For example: > > [nl219604 at skyvoice tmp] > $ echo -e "abc\0" > file1 > [nl219604 at skyvoice tmp] > $ echo "abc" > file2 > [nl219604 at skyvoice tmp] > $ gdiff -au file1 file2 > diff1 > [nl219604 at skyvoice tmp] > $ cat -v diff1 > --- file1 2009-02-16 18:12:15.208180647 +0800 > +++ file2 2009-02-16 18:12:24.246282178 +0800 > @@ -1 +1 @@ > -abc^@ > +abc > [nl219604 at skyvoice tmp] > $ /usr/bin/filterdiff diff1 2>errors >diff2 > [nl219604 at skyvoice tmp] > $ cat -v diff2 > --- file1 2009-02-16 18:12:15.208180647 +0800 > +++ file2 2009-02-16 18:12:24.246282178 +0800 > @@ -1 +1 @@ > -abc+abc > > So it can't output the newline character because of the previous NUL > character. > > I have fixed the bug, and generated the related patch file. Ok. > 3. There's an existing test suite in the package tarball, > but some changes need to be made to make to work on solaris. > Now all 126 test cases in the test suite behave as expected. > Should I also generated the patch files for the test suite? Since you have done the work and it will make it easier to test for you and others in the future and doesn't effect the pkg bins I would include the test patch files and contribute them to the patchutils community. > The updated webrev is located at: > > http://cr.opensolaris.org/~jason_li/patchutils/ I recommend you make the following changes: usr/src/cmd/patchutils/Makefile.sfw - create a Patches subdirectory, move your patch files to it, and add this line after the TARBALL define: 31 PATCHES:sh = echo Patches/*.patch Change this: 51 $(VER)/configure: $(TARBALL) 52 bzip2 -dc $(TARBALL) | $(GTAR) xopf - 53 touch $(VER)/configure 54 /usr/bin/gpatch $(VER)/src/util.c util.patch To: 51 $(VER)/configure: $(VER)/.patched 52 touch $@ _ then you can add patches without changing the Makefile.sfw. Meld does this: http://cr.opensolaris.org/~jwalker/meld/ usr/src/pkgdefs/SUNWpatchutils/Makefile - remove this line, since you have your own depend file and are not using the default depend file. 31 DATAFILES= depend > The updated the ARC materials are located at: > > http://greatwall.prc/~nl219604/package_porting/PSARC_materials/ > > Pls let me know if I can more onto the C-team review step. Yes. Please go on to the c-team step. Cheers, Jim
