Moriah Waterland wrote: > Mark, > > Here is the second half of my response. > > thanks, > Moriah > > Mark J. Nelson wrote: >> This is my last feedback. >> >> I explicitly did NOT review the source code, but concentrated on >> makefiles and packaging. >> >> >> >> usr/src/cmd/svr4pkg/pkgscripts: >> - What is "special_contents" used for? (Or is it unreferenced?) >> - None of the scripts specify an interpreter--I think they should. >> (#!/bin/sh) > It appears to just be unreferenced junk that is still laying around. I > removed the file (hg remove) and it didn't appear to cause any issues.
That looks like an answer about special_contents, but not about adding an interpreter line to your scripts? >> - Usually, if you want a script to be executable, you name it >> "blah.sh," and use the ".sh:" suffix rule to create "blah." You >> wouldn't even need to update the Makefile to accomplish this--simply >> rename the scripts, and it should work correctly. > Done, these files are actually just class action scripts. I was setting > the permissions because that was how it was handled in the Legacy > Install gate. After looking at other examples, I concluded that I do > not need to explicitly set the permissions for the scripts. I'll want to look at this in the followup webrev; sounds right. >> usr/src/lib/libinstzones/Makefile: >> usr/src/lib/libpkg/Makefile: >> _msg: Since you're already enumerating the object files in >> Makefile.com, and you're preprocessing for message extraction, it >> makes much more sense to handle _msg in Makefile.com, instead of >> here. And if you're really being clever, for libinstzones, you only >> need to extract from zones_strings.h, using the -a flag for xgettext, >> and not calling the preprocessor. > I ended up leaving this alone. I would have liked to have cleaned this > up, but I have already had way too much fun with these Makefiles and > have limited resources. Thank you for the advice and I will file a bug > on this next week. You didn't change anything about this? >> install_h: both of these makefiles have an install_h target, but >> they're not listed in HDRSUBDIRS in usr/src/lib/Makefile. If you >> don't plan to put the headers into the proto area, you can skip >> including Makefile.lib and Makefile.targ, and get rid of the HDRS and >> HDRDIR definitions. > Removed. Thanks. >> usr/src/lib/libinstzones/common/mapfile-vers: >> - please alphabetize > Done > >> >> It looks like you're adding lint libraries to the proto area, but I >> don't see where you're delivering them. If you plan for anything >> outside of ON to use them, they belong in SUNWarc. If not, they need >> to be in a packaging exception list, don't they? >> > I do not intend for anything outside of ON to use these lint libraries. > Is there a common exception list? I found: > o usr/src/pkgdefs/etc/exception_list_i386 > as well as > o usr/src/pkgdefs/etc/exception_list_sparc > I can add these libraries to both lists, but it seemed strange not to > see a common list. No common list, as you saw in separate thread. Thanks. --Mark
