See below ... Paul
Brian Utterback wrote: > http://cr.opensolaris.org/~blu/ntpv4/sfwnv-webrev/ > >> === Start of Comments ==== >> >> 1. usr/src/cmd/ntpd/METADATA >> This needs updating to conform to the heads-up of last week, >> as in ... >> "http://wikis.sun.com/display/SFWNotes/METADATA"; >> >> 2. tarball ntp-dev-4.2.5p161.tar.gz >> This is not in the webrev? > > No, should it be? It is the as downloaded. yes - otherwise how does it get into the gate on putback >> 3. version ntp-dev-4.2.5p161 >> The source pkg's website says there is version >> 'R.C.: 4.2.4p7-RC7 2009/05/12', so should you be using that >> now? > > No. I am shooting for 4.2.5, not 4.2.4. silly me, I should have read the numbers more carefully >> 4. usr/src/cmd/ntpd/install-sfw >> Some of the directories you are '${INSTALL} -d'ing, are >> already in the 'Targetdirs' file so you don't need to >> install then here. >> >> And the other directories created here could probably also >> be put in 'Targetdirs'. >> >> You could have used the '_install' macro rather than '${INSTALL}' >> for installing files. > > Didn't know that. I'll change it. What makes something appropriate or > inappropriate for Targetdirs? I think its anything that is likely to be used by more than one pkg. But historically in SFW it is all installed directories. >> 5. usr/src/cmd/ntpd/Makefile.sfw >> & usr/src/cmd/ntpd/install-sfw >> Line ... >> 69 (cd $(VER); env - $(CCSMAKE) all $(TARGET_ENV)) && \ >> install-sfw $(VER) >> is the 'make install' installing different stuff to >> 'install-sfw' ? > > No. "make install" just does a "make all" and install-sfw. There is no > "make install" done inside the untarred directory. whoops I read that wrong (read 'all' as 'install'). But having said 'whoops' shouldn't therefore .... the 'install-target:' rules depend on the 'all:' rule then you wouldn't need the '$(CCSMAKE) all $(TARGET_ENV)) &&' in .. 69 (cd $(VER); env - $(CCSMAKE) all $(TARGET_ENV)) && \ install-sfw $(VER) >> 6. Tops of files (various files) >> Cosmetic: Change so that they conform to those in ... >> "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/"; >> >> as per >> "http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines"; > > I am not sure what you are saying. The only files that don't have CDDL > and Copyright info are the Patch files. Looking at the other projects in > SFW, none of them have CDDL or copyright in the patch files. Or am I > misunderstanding you? I'm talking about the layout in the tops of your files - they are cosmetically different to the prototypes; and yes lots of those already in the sfw gate are wrong. But why propagate that wrongness. >> 8. usr/src/cmd/ntpd/Solaris/ntp >> Why '/sbin/sh' rather than '/usr/sbin/sh' ? > > Seems to the overwhelmingly predominant choice for SMF start-up methods. > I just followed the existing methods. okay fine with me >> 9. man pages >> Note, I haven't read their content >> >> 10. usr/src/cmd/ntpd/Solaris/ntp-keygen.1m >> & usr/src/cmd/ntpd/Solaris/ntpq.1m >> & usr/src/cmd/ntpd/Solaris/ntptime.1m >> & usr/src/cmd/ntpd/Solaris/ntptrace.1m >> Does this need the 'Interface Stability' stuff added? > > They are already all there. I don't use a script to add them. either I'm going blind or ..... 209 ATTRIBUTE TYPE^GATTRIBUTE VALUE 210 _ 211 Availability^GSUNWntp4u 212 .TE 213 .PP where's 'Interface Stability' ? >> And the where to find source added ? ... >> "Source for xxx is available on http://opensolaris.org > > Added to the METADATA file. its needed, I believe in the bottom of the man page also. > >> >> 16. usr/src/pkgdefs/SUNWntpu/copyright >> & usr/src/pkgdefs/SUNWntpr/copyright >> Does this need to include the lines 19 to 55 ? ? >> Should it also include a copy of the 'Modified BSD' >> licence (or are lines 5-15 that) > > Modified BSD is not longer accepted. It is now BSD-LIKE. > And, yes, 5-15 is the license. have you changed it in the METADATA then to "BSD-LIKE" then? >> 18. usr/src/cmd/ntpd/Solaris/ntprc.4 >> Have you delivered this in a SUNW pkg, I can't see where? > > It is a file format of an optional file, not delivered. so if its not used why not remove it - to stop confusion have you got an updated webrev ? -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Business Unit
