Apologies to individuals getting a second copy, the alias copy was rejected and this seemed more useful than creating a thread with a different cc list.
--Mark Moriah Waterland wrote: > Mark, Jim, and Sundar, > > Thank you for agreeing to review my changes related to move of the > SVR4 packaging code from the Legacy Nevada Install gate into ON. > > I am sure that I have already mentioned the following but here is a > quick overview: > > o This project is about moving the SVR4 packaging code to ON because > this code needs to live on and we need to move away from the > Legacy Install consolidation. > > o Patch code is not moving and is already dead in OpenSolaris. > David Comay is stopping the delivery of patch binaries in the > 2009.06 release. > > o The only goal of this project is moving the code. Yes, I know > that this means that problems are just moving from one place to > another but these issues must be dealt with separately. > > o Testing is on-going (Mary Ding has images she is working with and > I have additional tests I want to run etc..). I am reasonably > confident that this will not be an issue and I expect that if > changes need to be made they will be small. In other words, I > think that it is safe for you to start reviewing this now. > > I am cc'ing this code review to install-discuss at opensolaris.org as well > as several internal aliases. Please let me know if you think of other > aliases that this should also see this. Once this posts to > install-discuss, I will update the OpenSolaris project page for SVR4 > packaging with a link to this message. > > The bug report contains information about the reasons behind a lot of > the individual changes. I have also included it in this email for your > convenience. > > BugId: ( http://monaco.sfbay/detail.jsf?cr=6739234 ) > > 6739234 move SVR4 packaging to ONNV gate > > Webrev 1: This is a simple webrev of my changes to ON. It shows all the > new files that I am adding to ON as well as a the small number of > modifications that I made to several existing ON files. > > http://cr.opensolaris.org/~mwaterl/6739234.complete/webrev/ > > The first webrev shows all of the changes to ON but it doesn't show the > actual source modifications that I made to individual files. So I > generated a second webrev that should help highlight the actual source > changes that were required. > > > Webrev 2: This webrev was created by taking a clean ON child and adding > the files directly from the Legacy Nevada Install gate. These files > were inserted at their new locations. Then the updated files from the > SVR4 packaging move were added to the workspace so that reasonable diffs > could be generated. > > http://cr.opensolaris.org/~mwaterl/6739234.changes_only/webrev/ > > > Please get your feedback to me by COB on Friday, May 1st. Thanks again > for your help with this code review. > > -Moriah Please be sure to run "hg pbchk" on your repository, and fix what it reports. There are currently some copyright errors, and you'll want to whack keywords and make sure CDDL is up to date. It's OK to NOT update copyright for files that you're not changing, but please do that intentionally, and let your CRT Advocate know. usr/src/cmd/Makefile.cmd 62: Your ROOTUSRBIN is redundant with preexisting ROOTBIN. Basically, all of your changes to this file that do not involve ROOTUSRSADM are unnecessary, in this and other Makefiles. 328-330: You should not need this. ROOTUSRSADMSCRIPT should not refer to a directory, but instead to a list of files to be installed in the ROOTUSRSADM directory. usr/src/cmd/svr4pkg/Makefile 30 (old file): I'm fine with you moving your message into the SUNW_OST_OSCMD domain. Where was SUNW_PKG_CMDS.po (and friends?) previously delivered? Was there a SUNW0install or SUNW0adm package that you'll need to either modify or obsolete? 44: please alphabetize this list 65-83: please delete, and update usr/src/Targetdirs appropriately (note that "please delete" includes CPPFLAGS, since you're not using them here) 87-91: instead of 87 #all clean clobber lint: $(SUBDIRS) 88 all clean clobber lint: $(LIBSUBDIR) .WAIT $(CMDSUBDIRS) 89 90 #install: $(INSDIRS) .WAIT $(SUBDIRS) .WAIT _msg 91 install: $(INSDIRS) .WAIT all .WAIT _msg this section should read simply 49 SUBDIRS= $(CMDSUBDIRS) $(LIBSUBDIR) 87 $(CMDSUBDIRS): $(LIBSUBDIR) 88 89 all clean clobber install lint _msg: $(SUBDIRS) ...and line 89 should only include the lint target if it actually works correctly. (That's "correctly," not "cleanly," you already explained that this stuff isn't lint-clean.) Note that the definition of SUBDIRS will make lines 103-110 and 114-116 unnecessary. And you won't need a separate MSGSUBDIRS, since it's the same as SUBDIRS. _msg: - As implied by the suggested line 89 above, the install target should not depend on _msg. - The pkgname command ended up with no messages, so you could conceivably leave it out of MSGSUBDIRS, but it's not a big deal for it to be there. - instead of pkgcmds.po, please use svr4pkg.po to be consistent with directory name - add "include ../Makefile.targ" after the include for Makefile.msg.targ - remove 117, as it's covered by ../Makefile.targ 111-112: Just replace these lines with "CLOBBERFILES += $(POFILE)" Ok, dang, some duplicated effort here. I just got Jim's reply, and most (but not everything) of what I've said is in his note, too. Hitting "send," will continue under separate cover. --Mark > ======================================================================== > ======================================================================== > > > Suggested Fix > ======= > > The location changes for the source code are as follows: > Legacy Nevada Install gate: (The layout of the code in the old gate.) > o /ws/install-nv-gate/usr/src/ > o cmd/pkgcmds/ > hdrs/ > installf/ pkgcond/ pkgname/ > pkgrm/ pkgadd/ pkginfo/ > pkgparam/ pkgscripts/ pkgadm/ > pkginstall/ pkgproto/ pkgtrans/ > pkgchk/ pkgmk/ pkgremove/ > > o lib/libpkg/ > o lib/libspmizones/ > o lib/libinst/ > > o pkgdefs/SUNWpkgcmdsr > o pkgdefs/SUNWpkgcmdsu > || > || > \/ > Nevada ON gate: (The new layout for the code in ON.) > o /ws/onnv-gate/usr/src/ > o cmd/svr4pkg/ > hdrs/ libinst/ > installf/ pkgcond/ pkgname/ > pkgrm/ pkgadd/ pkginfo/ > pkgparam/ pkgscripts/ pkgadm/ > pkginstall/ pkgproto/ pkgtrans/ > pkgchk/ pkgmk/ pkgremove/ > > o lib/libpkg/ > common/ i386/ sparc/ > > o lib/libinstzones/ > common/ i386/ sparc/ > > o pkgdefs/SUNWpkgcmdsr > o pkgdefs/SUNWpkgcmdsu > > My changes for this code move only pertained to what was absolutely > necessary for this migration. I am aware that there are quite a few > other issues that could be fixed in this code, but they are out of scope > for this project. > > > Comments > ======= > The following notes are intended to provide more detail about why I made > certain changes: > > o Changed the name of the directory under cmd from pkgcmds to > svr4pkg to disambiguate this code from the new IPS packaging code. > > o Moved libinst out of libraries and into cmd/svr4pkg/libinst. Libinst > had compilation problems under the libraries directory in the ON > gate because it calls functions that are defined in code under > cmd/svr4pkg. Spoke with Dave Marker about situation and he suggested > this move because libinst is statically compiled, not delivered, and > will only be used by the package commands. > > o Leaving libinstzones (formally known as libspmizones) as NOT large > file aware. To track this, I filed: > 6833675 libinstzones is not large file aware > > o Created lint libraries for libinstzones and libpkg. Not linting > code under usr/src/cmd/svr4pkg. Changes required for this are out of > scope for this project. To track this, I filed: > 6833677 SVR4 package commands are not lint clean > > o I took changes to libinstzones directly from libinstzones in the > Caiman gate. This is where the library name was changed. > http://src.opensolaris.org/source/xref/caiman/slim_source/ > > o Moved the packages SUNWpkgcmdsr and SUNWpkgcmdsu from the install > gate to the ON gate. The only significant difference in what the > packages are delivering is the name change of libspmizones to > libinstzones. All references to libspmizones in the packaging code have > been changed to libinstzones. > > o Removed dependency from the packages for SUNWadmc as it is not required. > > o Used default text domain for the svr4pkg libraries or commands > because it didn't seem reasonable to define a separate domain just > for this old code. > > o Once this code is putback to ON, deliveries of SUNWpkgcmdsu and > SUNWpkgcmdsr from the Legacy Install gate will cease. The code will not > be yanked from the Legacy Nevada Install gate because the gate is in > maintenance mode until it is completely decommissioned.
