More comments below ... paul
Manjunath Basappa wrote: > > Please find the new set of webrev files for this project at : > http://cr.opensolaris.org/~manjuhb/routes/ > > Please find my comments embedded in the email. > > Paul Cunningham wrote: >> Manjunath Basappa wrote: >>> Webrev files are located at http://cr.opensolaris.org/~manjuhb/mb117111/ >> >>>> >>>> Please see the new code review webrev files. I have incorporated >>>> the corrections you have mentioned. >>>> >>>> Note, we are now delivering two packages, for py 2.4 and py 2.6 >>>> from the same source file. >>>> >>>> Please review and let me know your feedback. >> >>>>> >>>>> Manjunath Basappa wrote: >>>>>> >>>>>> I am porting routes package to opensolaris /release repository. >>>>>> >>>>>> Please find attached the relevant documents. >>>>>> >>>>>> I also request a review of the source code. The webrev output is >>>>>> located at http://cr.opensolaris.org/~manjuhb/webrev/. 1. usr/src/lib/Routes2.4-bak/* I don't think these should show up in your webrev ? 2. usr/src/lib/Routes/Makefile.sfw Lines 48-52 (install: rule), shouldn't the 'protofix' be done after you have installed into the ws proto area?, ie. move line 49 to after line 52 >> 3. usr/src/lib/Routes/sunman/py-routes.3 Line 65, I think it is more normal to have something like .. "Source for Routes is available on http://opensolaris.org."; I don't think you need the CDDL header any longer, see ... Jim Walker wrote: >> 2. usr/src/cmd/meld/meld.1 >> Is there a reason why you have taken out the CDDL header stuff > > The sfw cteam has decided that man pages should use the > same license as the package being ported. Thus, no > CDDL headers anymore. >> 3A. usr/src/pkgdefs/Makefile This needs resyncing with the gate/clone so it doesn't look as though you are changing other stuff >> 4. usr/src/lib/Routes2.4-bak >> What is this stuff in the webrev? > Removed. - its still in the webrev >>>>> 10. usr/src/pkgdefs/SUNWpython24-routes/pkginfo.tmpl & 26 >>>>> Add the pkg version to the end of the DESC= line, eg... >>>>> DESC="............. (1.10.3) >> >> - put the version at the end of the line as above - put the version at the end of the line to be consistant with other packages (as above) 10A. SUNWpython26-routes/pkginfo.tmpl Typo ... 43 DESC="Routes(version 1.103) >> 12. SUNWpython24-routes/prototype_com >> & SUNWpython26-routes/prototype_com >> & Routes/Makefile.sfw >> I still don't like that you are delivering the same files >> twice in two different packages. >> Just a thought: Could one be a symbolic link >> to the others files, ie. the 24 files be a symbolic link to >> the 26 file, the symbolic links being set up in >> SUNWpython24-routes/prototype_com and 24 depend on 26 ? > > The only files which are common in SUNWpython24-routes and > SUNWpython26-routes is Makefile > and copyright file. > > I thought it is better to have separate Makefile rather than have a link. I was referring to the files that you actually delivering in the two pkgs, ie. those listed in the two prototype_com files. For example, the files ... - usr/lib/python2.4/vendor-packages/routes/base.py - usr/lib/python2.6/vendor-packages/routes/base.py etc. are the same file (as copied in Makefile.sfw .... 52 cp -R * $(REALPROTO26); cp -R * $(REALPROTO24)) ). so pkg SUNWpython24-routes could just set up symbolic links to the files from SUNWpython26-routes and SUNWpython24-routes/depend just depend on SUNWpython26-routes. - but this is just a thought, and there may be reasons why it can't. END
