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 >>> >> >
