Paul, Thank you for your patience in dealing with me!
I have made all the changes you have suggested except the last one, creating links. I just thought, what if someone installs routes 2.4 on a machine where only python 2.4 is installed...so, may be it is a good idea if we keeps the files separate. Correct me if I am wrong.. And the webrev location is again the same.. http://cr.opensolaris.org/~manjuhb/routes/ -manjunath Paul Cunningham wrote: > 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
