Paul Cunningham wrote: > Daria, > > See below for my comments from my skip through ... Paul,
Thank you for thorough review. Being new to the task, I knew there would be plenty of details I didn't get right. See comments inline, and the new webrev here: http://cr.opensolaris.org/~dmehra/libnet/ Note, the webrev also has changes to libnet_link_dlpi.c which address comments from Peter Memishian: > The /dev/net nodes will always be DLPI style 1 (that is, they will always > end in a number). Please simplify accordingly. Also, why are there > unnecessary changes such as the whitespace on line 55? Simplified to look up only DLPI style 1 in /dev/net, and removed superfluous diffs. The rest of inline responses address Paul's review. I made most of the recommended changes, but had trouble with item (8), and not sure what was meant in (10), please see below. > > 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. Added. > > 2. usr/src/pkgdefs/SUNWlibnet/Makefile > Copyright year is wrong Changed year to 2008. > > 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. Removed libnet's own depend file, it will be using the default. > > 4. usr/src/pkgdefs/SUNWlibnet/pkginfo > Delete this file as its autogenerated from the > pkg's pkginfo.tmpl file. Deleted pkginfo. > > 5. usr/src/pkgdefs/SUNWlibnet/pkginfo.tmpl > Expand the 'DESC=...' line more to add a better > pkg description. Done, is the new description good enough? I kept it short because other packages seem to do so. > > > 6. usr/src/pkgdefs/SUNWlibnet/prototype_com > Do the files in usr/include/libnet need to have the > 'write' and 'execute' permissions set? No, good catch, corrected to 644. > > 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). Done. > > 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). This approach is not working for me with bash. I see librsync using this with ksh93 (not a shell I'm used to), but bash gives an error for "set -o errexit" line: /usr/bin/sh ./install-sfw ./install-sfw: -o: bad option(s) Also, if I use "source" instead of the dot operator to source the install.subr file, bash doesn't find it: /usr/bin/sh ./install-sfw ./install-sfw: source: not found ./install-sfw: _install: not found To the best of my understanding, "source" and "." are equivalent in bash. Do they behave differently if errexit option is set? If this behavior is desired, I'll rewrite the install scripts in ksh93 to make it work... or perhaps someone can send an example of how do it in bash. A higher-level question is where to find documented guidelines on this? Few packages in the consolidation are following the above, so I'm assuming it's a new rule. I am using these docs, and none of them mention this (unless I failed to follow several levels of links somewhere): http://ostest.central.sun.com/wiki/index.php/Package_Delivery_Project http://webtier.sfbay.sun.com/webtier/attach/ApacheTop/IntegratingIntoSFW.html http://sfwnv.sfbay/README.txt > > I don't thing 'ROOTMAN3DIR=..' is used so remove? Removed. > > I don't think you should have defined .. > 'ROOT=/builds1/dm155201/tmpinstall' as it should > be pointing at the ws proto area? Sorry, I had created the webrev with my test version of some files. Fixed that up (and now I understand I didn't need to hack up those files for testing). > > Remove the 'echo' lines. Done. > > The '${INCDIR}/libnet/*' file permissions don't > match those in the prototype_com file - see (6). > The 'man' files don't either. Fixed header permissions to 644 and man files to 444. > > You need to add the Sun stability stuff to the > 'man' pages - see other pkg's for example of how > to do that. Added "Sun stability stuff" to the best of my understanding, based on other projects' examples. However I'm not sure which interface level to advertise here: libnet's own (Volatile) or of SUNWlibnet package (Uncommitted). I must confess I do not fully understand the stability classification. The ARC case was approved like this, please advise which classification to put into the sunman-stability file. 4.1 Exported Interfaces Interface Name Classification Comments --------------------------- ------------------- --------------------------- libnet Volatile version 1.1.2.1 SUNWlibnet Uncommitted libnet's packaging > > > 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) Ok, I made this change, but I don't see any of the already integrated libraries doing "env -" in their makefiles. Is this another new rule? > > > > 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). Done; as with above point, I only see a couple of projects who did it this way, is this a new rule too? > > Use the predefined 'configure --prefix' defined in > Makefile.master, and invoke 'configure' with > './configure $(CONFIGURE_OPTIONS) - see other > recent pkg integrations for examples. Thanks, that resolves my prefix confusion. Now, in order to test "make -f Makefile.sfw install" of my package, I end up modifying Makefile.master to point to my private ROOT. Is this the right approach? I'd rather set an environment variable, but that doesn't seem to be supported. > > 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)'. Fixed "all" rule, LDFLAFS bracket, and spaces, but double quotes are only used as needed, consistent with other projects' Makefile.sfw. > > Combine the two clean 'rm' lines into one. Done, but existing Makefile.sfw of other projects have two lines, I tried to follow the convention. > > 10. usr/src/lib/libnet/install-sfw-64 > See Roland Mainz comments in (8). Addressed. > > Line '42 _install D src/libnet.so ${LIBDIR}/libnet.so 755' > I think you have also installed this in > 'install-sfw' ? Yes... 'install-sfw' puts in the 32-bit version of the library, and this is the 64-bit version. Am I misunderstanding the way install scripts are meant to work? I followed the example of projects such as flex and libpcap. Install seems to do the right thing in my testing. > > === End of Comments ===== > Thanks, -- daria
