Paul, Response inline. Thanks for your scrutiny. - Simon
Paul Cunningham: > Simon, > > More comments ... > > Paul > > lingjun wang (Simon) wrote: >> >> See my inline comments. >> >> Thanks very much for your review. >> >> Paul Cunningham : >>> >>> Mainly looks good to me, just few minor comments ... >>> >>> >>> Ling-Jun Simon Wang wrote: >>> >>>> <a href=http://cr.opensolaris.org/~lw202042/smartmontools/>code >>>> review for smartmontools</a> >>>> >>> >>> >>> ====== Start of Comments ================ >>> >>> 1. usr/src/pkgdefs/Makefile >>> You seem to have removed some apache stuff, did u mean to? >>> >> Actually I didn't remove any stuff here. Instead, I need a bringover >> and I did it. >> >>> 2. usr/src/cmd/smartmontools/smartmontools.xml.in + >>> usr/src/pkgdefs/SUNWgnu-smartmontools/depend >>> You might want to move the copyright lines to after the >>> "CDDL HEADER END" >>> >> Yes. I did it. >> >>> 3. usr/src/pkgdefs/SUNWgnu-smartmontools >>> You don't seem to have included the Makefile in the webrev >>> >> Yes. I include it now. > > SUNWgnu-smartmontools/Makefile > I don't think you need the 'DATAFILES = depend' line if you have your > own 'depend' file. Yes. I removed it. > >>> 4. usr/src/pkgdefs/SUNWgnu-smartmontools/pkginfo.tmpl >>> You might want to remove the package version from the >>> 'DESC=' line. >>> >> Yes, I removed it. >> >>> 5. usr/src/pkgdefs/SUNWgnu-smartmontools/prototype_com >>> Should the 'etc' stuff be in a root package? >>> >> It should be in the root package. > > What I was trying to get at is that the following should be in a > separate SUNWgnu-smartmontoolsr (root) package I think (maybe) .. > d none etc 0755 root sys > f none etc/smartd.conf 0644 root sys > and maybe .. > d none lib 0755 root bin > d none lib/svc 0755 root bin > d none lib/svc/method 0755 root bin > f none lib/svc/method/svc-smartd 0555 root bin > d none var 0755 root sys > d none var/svc 0755 root sys > d none var/svc/manifest 0755 root sys > d none var/svc/manifest/application 0755 root sys > f none var/svc/manifest/application/smartmontools.xml 0444 root sys > > but check elsewhere But a separate root package seems to be over complicated in my humble opinion. > >>> 6. usr/src/pkgdefs/SUNWgnu-smartmontools/prototype_sparc + >>> usr/src/pkgdefs/SUNWgnu-smartmontools/prototype_i386 >>> name wrong in file - # SFWgnu ???? >>> >> Yes, a typo. I corrected it. >> >>> ====== End of Comments ================== >>> >
