Hi Jim, Jim Walker ??: > 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. > OK, I've generated the patch file, and added the corresponding target into the Makefile.sfw file. I will send a bug report the package author. > >> 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 > I have updated the webrev, please have a look at: http://cr.opensolaris.org/~jason_li/patchutils/ > > >> 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. > OK, thanks.
-Jason > Cheers, > Jim > >
