Hi Amanda, webrev updated again. Now all the things should be in line. Thanks, Milan
Dne 1.06.09 14:56, Amanda Waite napsal(a): > Milan Cermak wrote: >> 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. > > As I pointed out in the free impi review, reviewers are not going to > approve a webrev which isn't synced with the gate/clone. > > >> >>> 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. > > $(COMPONENT_NAME:sh) uses the METADATA NAME field. It should be just the > name of the component as a single word > >> >>> - 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). > > Yeah, you're right. Brain fade on my part. > >>> 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. > > You can never make wx nits happy, bet you can't add a CDDL header to the > source tarball ;o) > >> >>> 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. > > Update the package string in prototype_* in the renamed package, it's > still SUNWfluid-soundfont > >> >>> - 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). > > Lots of packages are just data, they all either have the default depend > file or a depend file supplied by the integrator that's based on the > default. Every package in SFW is this way. > >>> 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. > > See comments above. > >> >>> 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. > > See comments above. > > > >> >>> 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 *
