Thanks Amanda, I fixed most of the points and updated the webrev. I comment on others below.
Webrev: http://cr.opensolaris.org/~mcermak/webrev/ Regards. Milan Cermak Dne 29.05.09 12:16, Amanda waite napsal(a): > 1. usr/src/cmd/Makefile & usr/src/pkgdefs/Makefile > > You need to resync with the clone/gate as there are changes there that > you don't have in your workspace which means you are effectively > removing stuff :o/ Done, but this will change with each putback to sfwnv. I plan to synchronize just before real putback. > 2. usr/src/cmd/fluid-sf/METADATA & usr/src/cmd/timidity/METADATA > > http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines I don't see what to change here. I made METADATAs according the wiki page originally. > 3.* *usr/src/cmd/fluid-sf/Makefile.sfw* > > *Once you've done 2 you can use the METADATA fields to build $(VER). > Look at other components in your workspace to see how it's done. Done as much as possible. > 4. usr/src/cmd/fluid-sf/install-sfw > > Remove the mkdir line and add the directory to usr/src/Targetdirs Done. > 5. usr/src/cmd/timidity/Makefile.sfw > > - Again, build as much of the $(VER) as you can using the metadata, save > having to change it in multiple places Done. > - In TARGET_ENV you don't specify DESTDIR, I can't check at the moment > but I assume DESTDIR is set for TARGET_ENV in the build environment. > Just checking. DESTDIR changes root for installation. It is used only in 'make install' phase and that is not used in sfwnv (replaced by install-sfw script). > 6. usr/src/cmd/timidity/install-sfw > > - You shouldn't need to create any directories, particularly not the > standard paths such as usr/bin and usr/share/man/man1. f you're testing > on a clean workspace, add new directories that your package is adding to > usr/src/Targetdirs and run 'make setup' in usr/src. You can then build > and 'make install' your component Fixed. > 7. usr/src/cmd/timidity/sunman-stability > > - You don't need a CDDL header for this file But it doesn't hurt and makes wx nits happy. > 8. usr/src/pkgdefs/SUNWfluid-soundfont/Makefile > > - Was it agreed in ARC to call this package SUNWfluid-soundfont and not > SUNWfluid-soundfontu? You have a 'root' package with an 'r' suffix, how > comes the 'usr' package has no 'u' suffix? SUNWfluid-soundfont renamed to SUNWfluid-soundfontu. > - If you aren't supplying a depend file then you want the build to > provide one, add > > DATAFILES= depend > > after the include line. I don't supply depend file because fluid soundfont is just a data. It provides no executables. Timidity makes use of it but can use other soundfont as well. And soundfont may be used by other applications (hence no dependence on timidity). > 9. */copyright files > - Have you included all of the copyright strings from all of the > source/header files? Yes. > 10. */pkginfo.tmpl files > - Include the version number in parens at the end of the DESC field Done. > 11. usr/src/pkgdefs/SUNWfluid-soundfont/prototype_com > - You want to include the depend file (see comments on package Makefile) > so add 'i depend' after 'i copyright' I don't. Fluid SoundFont is just a data. > 12. usr/src/pkgdefs/SUNWfluid-soundfontr/depend & > usr/src/pkgdefs/SUNWtimidity/depend > - You need to include dependencies on the core packages, copy the > default depend file from the workspace into your pacakge dir and add > your dependencies to that. I think the file is .. > usr/src/pkgdefs/common/depend or something like that. Do I? The way they are, depend files include just the necessary dependencies. No more, no less. Package manager should perform a transitive closure on dependencies and I'm sure, it does. > Amanda > * > > ** > *Milan Cermak wrote: >> Just a friendly reminder. Can I have a review, please? >> Milan >> >> Dne 25.05.09 14:13, Milan Cermak napsal(a): >>> Hi all, >>> I'd like to ask for a code review of integration files of TiMidity++ >>> to SFW. >>> >>> Webreview: http://cr.opensolaris.org/~mcermak/webrev/ >>> >>> Thanks, >>> Milan Cermak >> > -- * The only position I can honestly require is DLab - Data Labourer *
