Do you really need the write bit set on the files in usr/src/pkgdefs/SUNWtimidity/prototype_com? If not remove it from them, makes it harder to break the package with an inadvertent overwrite. Otherwise it looks good.
Amanda Milan Cermak wrote: > 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 >>>>> >>>> >>> >> >
