Liu Xin, See below for my comments, note I'm a bit confused at why you are doing somethings as you are ...
Paul Liu Xin wrote: > > Please help review the fix of CR6689922 CR6739050 and CR6739053. Any comments > are welcome. Thanks. > > Webrev is at: > http://cr.opensolaris.org/~liuxin/ocaml_lablgtk_unison/ > <http://cr.opensolaris.org/%7Eliuxin/ocaml_lablgtk_unison/> > > CR synopsis: > > 6689922 <http://monaco.sfbay.sun.com/detail.jsp?cr=6689922> unison 2.27.57 to > be included into SFW consolidation > 6739050 <http://monaco.sfbay.sun.com/detail.jsp?cr=6739050> ocaml-3.10.2 to > be included into SFW consolidation > 6739053 <http://monaco.sfbay.sun.com/detail.jsp?cr=6739053> lablgtk-2.10.1 to > be included into SFW consolidation > > PSARC case: > http://opensolaris.org/os/community/arc/caselog/2008/212/ > http://opensolaris.org/os/community/arc/caselog/2008/406/ === Start of Comments ==== 1. usr/src/cmd/Makefile Shouldn't unison have a build dependency on the ocaml compiler build? For lablgtk see comment (5) 2. usr/src/cmd/unison/Makefile.sfw Why are you moving stuff about that belongs to ocaml and lablgtk in this Makefile (lines 48-52), shouldn't that have been done in the builds of those packages at the appropriate time? 3. usr/src/cmd/ocaml/Makefile.sfw Why are you 'configure'ing this twice? My guess is that you somehow want to be able to use the ocaml you put into the proto area to build unison - but the proto area should have the real SUNWocaml files in it. Use the default 'configure --prefix=..' value defined in Makefile.master (and any other defaults). You could also use the CONFIGURE_OPTIONS method, eg. './configure $(CONFIGURE_OPTIONS)' see other recent pkg integrations. 4. usr/src/cmd/ocaml/install-sfw Add 'line space' after line 45 ('make install' line). Is this 'make install' actually installing it into the proto area (eg. no DESTDIR= or equivalent)? Do all these files need the write permission bit? 5. usr/src/cmd/lablgtk/Makefile.sfw Why are you building ocaml again in this lablgtk Makefile ? Shouldn't you be using the one that was built by ocaml/Makefile.sfw ? 6. all install-* files Look back in sfwnv-discuss at 'Roland Mainz' comments on other peoples install-* files and see if these are appropriate to yours. 7. usr/src/cmd/lablgtk/install-sfw Is the 'make install' actually installing it into the proto area (eg. no DESTDIR= or equivalent)? Do all these files need the write permission bit? 8. usr/src/cmd/lablgtk/ocaml-3.10.2.tar.gz & usr/src/cmd/ocaml/ocaml-3.10.2.tar.gz Why put this source tarball twice in gate? 9. usr/src/pkgdefs/SUNWunison/pkginfo.tmpl Expand the DESC= so that its more descriptive 10. usr/src/pkgdefs/SUNWocaml/copyright Are the authors listed in here really the same as in SUNWunison/copyright ? And in SUNWlablgtk/copyright 11. usr/src/pkgdefs/SUNWocaml/pkginfo.tmpl Expand the DESC= so that its more descriptive 12. usr/src/pkgdefs/SUNWlablgtk/depend Does it really depend on SUNWocaml, the ocaml compiler? 13. usr/src/pkgdefs/SUNWlablgtk/pkginfo.tmpl Expand the DESC= so that its more descriptive 14. usr/src/pkgdefs/SUNWlablgtk/prototype_com Should you be delivering the static *.a libraries? There are also some in SUNWocaml/prototype_com. === End of Comments ====== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Computer Products
