<comments in-line>
Mark J. Nelson wrote:
> Moriah Waterland wrote:
>> I have generated a new webrev for: 6739234 move SVR4 packaging to
>> ONNV gate
>>
>> This webrev includes the changes that I detailed in my responses to
>> code review comments.
>>
>> Updated Webrev (2nd ed.):
>> [http://cr.opensolaris.org/~mwaterl/6739234.changes_only.2/webrev/]
>>
>>
>> Old Webrev:
>> [http://cr.opensolaris.org/~mwaterl/6739234.changes_only/webrev/]
>>
>> Jim, Mark, and Sundar: I would appreciate an explicit "ok" from
>> each of you before noon(PT) on Tuesday, May 26th.
>>
>> thanks, Moriah
>
> I used the .3 version of the webrev.
>
> usr/src/cmd/Makefile: - Looks like you lost the change adding svr4pkg
> to MSGSUBDIRS?
Fixed. Added svr4pkg back to MSGSUBDIRS target.
> usr/src/cmd/Makefile.cmd: 314: Where is ROOTPKGSCRDIR defined? 483:
> Where is ROOTVARSADM defined?
Removed rules. *sigh* Cruft that shouldn't be there anymore.
> usr/src/cmd/svr4pkg/Makefile: - Has something gone wonky with the
> workspace? This isn't the makefile that belongs here, it's the one
> that should be in pkgtrans, isn't it?
Arrgh. Yes, when I was copying all the pkg directories I accidentally
copied the contents of pkgtrans to the wrong level and forgot to clean
up this Makefile.
> usr/src/cmd/svr4pkg/Makefile.svr4pkg.targ: 40: You add $(PROG) to
> $(CLOBBERFILES) in Makefile.svr4pkg, so you shouldn't need to list it
> separately here.
Done. I removed $(PROG) from the clobber rule.
> - I thought that you planned to leave the lint target in place?
No, I decided to remove it because I couldn't include the default
targets from cmd/Makefile.targ and would thus need to redefine them in
my Makefiles. This is related to:
6843647 $(MSGDOMAINPOFILE) target should be cleaned up under usr/src/cmd
I filed a bug regarding adding this target once 6843647 is fixed:
6846516 clean up _msg targets under svr4pkg and add default lint targets
> usr/src/cmd/svr4pkg/libinst/Makefile: 32: I had previously suggested
> that ARFLAGS should be "r," rather than "cq." I couldn't find a
> reply from you saying why you chose this, but I also couldn't seem to
> find the "part 1" of your reply to my comments.
I removed that line and am just using the default setting for ARFLAGS
which is -rv
The -c and -q options were used in the legacy gate, but using them seems
to be pretty silly:
-c Suppresses the diagnostic message that is written to
standard error by default when archive is created.
-q Quickly appends files to the end of archive. Position-
ing options -a, -b, and -i are invalid. The command
does not check whether the added files are already in
archive. This option is useful to avoid quadratic
behavior when creating a large archive piece-by-piece.
> 54-56: I had previously commented that this, too, seemed
> wrong--there's a ".l.i" suffix rule in usr/src/Makefile.master, which
> should make this unnecessary. If (when?) you fix this, you'll want
> to fix your clean target, too, to get rid of this .c file.
Fixed. I misunderstood your previous comment. I also fixed the clean
target and am no longer adding the .c file as the ".l.i" and ".l.o"
targets already remove it.
> 67: Why are you redefining CSTYLE?
Removed this definition. It was in the file over in the Legacy Install
gate and just had not been removed.
> usr/src/cmd/svr4pkg/pkgscripts/Makefile: 46-49: Why bother to rename
> this to correspond to the directory? You're already getting
> "cmdexec.po" from Makefile.svr4pkg, that should still work.
> Especially since you're NOT extracting messages from the scripts.
Fixed. And yes, that is a good point:)
> 51-52: These are no longer necessary; they're in Makefile.svr4pkg.
Removed lines 51 and 52.
> 59-61: Order should be swapped; "all" should be default target. - I
> thought you had agreed to follow my earlier suggestion to rename the
> scripts with ".sh" extensions? In which case they should also be
> dependencies of "all"?
Fixed. All is the default target. This I also misunderstood what you
were asking for with the .sh extensions. I have fixed this and renamed
all of the scripts to *.sh.
> usr/src/lib/libinstzones/Makefile: - I thought you had decided to
> pass "_msg" to SUBDIRS, instead of building it here. If you do it
> here, then you must also deal with clean and clobber of the .i files
> and POFILE, respectively.
Done. I just moved dealing with _msg to Makefile.com and am already
dealing with clean and clobber there. I also closed the bug I had filed:
6842322 cleanup _msg targets in Makefiles for libpkg and libinstzones
as not reproducable.
> usr/src/lib/libpkg/Makefile: - Same comment about deferring _msg
> build, and/or cleaning up after it. 28-29, 52: This doesn't make any
> sense unless you add libpkg to HDRSUBDIRS in usr/src/lib/Makefile.
Removed lines.
> 66: Since you're appending to CPPFLAGS, you should not add
> CPPFLAGS.master here.
Done. I removed CPPFLAGS.master from line 66.
> usr/src/lib/libinstzones/common/mapfile-vers:
> usr/src/lib/libpkg/common/mapfile-vers: - SUNWprivate is not usually
> versioned; why is that done here?
Fixed. I removed the version info. When I had looked at other examples
of mapfile-vers under usr/src/lib most of them had a versioned
SUNWprivate definition, so I incorrectly assumed that was proper.
> usr/src/lib/libpkg/THIRDPARTYLICENSE: - Did you talk to Jim about
> this? You need this here just because you link to OpenSSL, or is
> there additional code? The copyrights in p12lib.[ch] are accurate?
Yes, these should be accurate. This code has already been through the
due diligence process and was released on opensolaris.org a couple years
ago.
> usr/src/pkgdefs/Makefile: - 199-200: please alphabetize
Done.
> usr/src/pkgdefs/SUNWinstallint/pkginfo.tmpl: - You should still
> define the zones-related parameters, even for an internal package.
Done. I added the following definitions:
SUNW_PKG_ALLZONES=false
SUNW_PKG_THISZONE=false
SUNW_PKG_HOLLOW=false
> usr/src/pkgdefs/SUNWpkgcmdsr: 36: You're naming classes that you're
> not actually using in this package. Please remove logadmconf and
> manifest from CLASSES.
Done.
> usr/src/pkgdefs/SUNWpkgcmdsu/prototype_com: 76: please alphabetize
Done. This is a legacy file. However, I alphabetized it since it
didn't involve much effort and made it a lot easier to read.
> usr/src/pkgdefs/SUNWpkgcmdsu/pkginfo.tmpl: 37: Should be
> "ONVERS,REV=0.0.0"
Done. I changed line to be:
VERSION="ONVERS,REV=0.0.0"
I am updating the webrev and will be sending the final version out later
today.
thanks,
Moriah
