Daria, See below for my comments from my skip through ...
Paul Daria Mehra wrote: > Please provide code review comments for libnet's integration into SFW. > See my webrev here: > > http://cr.opensolaris.org/~dmehra/libnet/ > > CR 6687609, PSARC/2008/409 was approved. Note that there are code > changes (libnet_link_dlpi.c) which will not be integrated back into > upstream community; I would appreciate a careful look at those. Also, > this is my first experience with SFW consolidation, and I may have > missed some pieces through unfamiliarity with the process. === Start of Comments === 1. METADATA You need to add a METADATA file; see other recent integrations for examples, eg. cups. 2. usr/src/pkgdefs/SUNWlibnet/Makefile Copyright year is wrong The 'DATAFILES= depend' say you want to use the default 'depend' file, but you have your own one checked-in. See (3) 3. usr/src/pkgdefs/SUNWlibnet/depend This looks like the default 'depend' file, if you have no other dependencies of your own then you can remove this file. But have you checked you have none of your own with the dependency checker script. Also see (2). If you keep this file move the 'Copyright' lines to after the 'CDDL HEADER END' header and correct year. 4. usr/src/pkgdefs/SUNWlibnet/pkginfo Delete this file as its autogenerated from the pkg's pkginfo.tmpl file. 5. usr/src/pkgdefs/SUNWlibnet/pkginfo.tmpl Expand the 'DESC=...' line more to add a better pkg description. 6. usr/src/pkgdefs/SUNWlibnet/prototype_com Do the files in usr/include/libnet need to have the 'write' and 'execute' permissions set? 7. usr/src/pkgdefs/SUNWlibnet/prototype_i386 You might want to add the SUNW pkg name comment line as in your prototype_sparc file (no that it matters). 8. usr/src/lib/libnet/install-sfw Roland Mainz wrote: > Please use /usr/bin/ksh93 or /usr/bin/bash for > install-sfw* and add a > $ set -o errexit # at the beginning and replace ". > ${SRC}/tools/install.subr" with > "source ${SRC}/tools/install.subr" (the idea is > to catch failures in the script and abort it at > that point, right now the script will just continue). I don't thing 'ROOTMAN3DIR=..' is used so remove? I don't think you should have defined .. 'ROOT=/builds1/dm155201/tmpinstall' as it should be pointing at the ws proto area? Remove the 'echo' lines. The '${INCDIR}/libnet/*' file permissions don't match those in the prototype_com file - see (6). The 'man' files don't either. You need to add the Sun stability stuff to the 'man' pages - see other pkg's for example of how to do that. 9. usr/src/lib/libnet/Makefile.sfw Roland Mainz wrote: > Please use "env - ..." and not "env ..." in the > Makefiles to make sure "configure"&&"make" only > see the env variables they should really get > (and not pick-up any random env variable) > > Please use either $(SHELL) or /usr/bin/bash for > "configure" calls (so we know which one is used > and "configure" doesn't pick one itself). Use the predefined 'configure --prefix' defined in Makefile.master, and invoke 'configure' with './configure $(CONFIGURE_OPTIONS) - see other recent pkg integrations for examples. You could change the '36 all : real-all' rule to ' all: all32 all64' and remove the 'real-all:' rule. '69 "LDFLAGS = $LDFLAGS)" \ - looks like a missing open bracket '(' ? Be consistent with 'X = $(X)' and 'X=$(X)', ie. spaces. Be consistent with quotes - '"X=$(X)"' and 'X=$(X)'. Combine the two clean 'rm' lines into one. 10. usr/src/lib/libnet/install-sfw-64 See Roland Mainz comments in (8). Line '42 _install D src/libnet.so ${LIBDIR}/libnet.so 755' I think you have also installed this in 'install-sfw' ? === End of Comments ===== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Computer Products
