On 06/ 7/11 09:00 AM, Danek Duvall wrote: > Alan Coopersmith wrote: > >> http://cr.opensolaris.org/~alanc/7016849/
Version 2: http://cr.opensolaris.org/~alanc/7016849-v2/ > Makefile: > > - It would be useful to have rules that downloaded new ID files. You > could store the URLs in the makefile and have something like > > pci.ids: > curl -s -o $@ $PCI_URL > > and force regeneration with "gmake -B pci.ids". Done. > - line 41: can you put a definition for GSED (or SED if you really want, > but either way, I don't see a point in using the Solaris version) in > shared-macros.mk and use it? Points for fixing all the other > components up. GSED added to shared-macros.mk - I'll have to look through the other places sed is called, but haven't touched any others in this change. > - line 51: why use both basename and F? $(basename) in gmake is not quite the same as the shell command basename. $(basename $F) expands to ".../components/hwdata/build/pci.ids" $(@F) expands to "pci.ids.license" $(basename $(@F)) was the simplest way I found to get just "pci.ids" Added comments to the Makefile explaining for future reference. > - line 52: I'm confused by the use of the curly braces. Don't they need > to be paired? Or do the -e arguments simply concatenate? And if so, > why not just do it in the makefile? Each -e argument becomes a new line of input to the sed script, so sed effectively sees a single script of: s/__FILENAME__/$(basename $(@F))/ /__HEADER__/ { r $< d; } Unfortunately, the 'r' command in sed to read in a file uses the rest of the line as the filename so requires a line break (either explict or implicit from -e) before the d command to delete the __HEADER__ line. This could also be shown with sed ... -e '/__HEADER__/ { r $<\ d; }' but the two -e's looked less messy to me. (I had to read the sed faq to figure out why my original '{ r $< ; d ; }' was not working, and this was the suggested solution.) This could also be done as two match/action pairs instead of one with braces: $(SED) -e 's/__FILENAME__/$(@F)/' \ -e '/__HEADER__/ r $<' -e '/__HEADER__/ d' ids.license.tmpl > $@ I've changed to that so it's hopefully less confusing to those who haven't read the sed faq, and tried to comment sufficiently. (Probably nanoseconds less efficient doing the match twice, but if your build system is so slow that doing an extra match per line matters on a 38 line text file, you've got much worse problems.) > - I'd suggest swiping the date from the ids files and stuff that into the > version, but since they're maintained independently, you'd need > separate packages, which doesn't seem worth the trouble. Yeah, that's why I hadn't, since the usb.ids & pci.ids update at different rates, so wouldn't have the same date/version. > *.p5m: > > - We don't usually use the whole fmri stem in the manifest filename; > hwdata.p5m would be sufficient. > > - line 43: is there any particular reason that this is group sys, > particularly when the files aren't? Both of those are simply due to copying the files from ON - I've renamed the manifest to hwdata.p5m. I assume group=sys was used on the directory to match the usr & usr/share directories (which get group=sys via transforms/defaults ), and since it's already that way in the existing packages/filesystem, just left it. -- -Alan Coopersmith- [email protected] Oracle Solaris Platform Engineering: X Window System _______________________________________________ on-ips-dev mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/on-ips-dev
