Here's more Makefile feedback for the stuff under usr/src/cmd/svr4pkg. It's not complete, but I have a 4:15PM appointment.
--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 usr/src/cmd/svr4pkg/libinst/Makefile: General: For libraries that build under usr/src/cmd, you might still want to include $(SRC)/lib/Makefile.lib et. al. Then you can avoid a bunch of the stuff that you're redefining here. In particular, look through usr/src/lib/Makefile.{lib|targ}, and how they build the $(LIBRARY) target. (Which is not usually done, but is still supported by the infrastructure.) I really echo Jim's concern that this SHOULD be a dynamic library. The following is not a comment about you, personally: In my experience, if this integrates as-is, nobody will ever go back and fix the "shoulda-coulda-woulda done it differently if only we had more time" problems. 28: If you're not using the library makefiles, and not creating a dynamic library, then I don't see where you're using $(VERS). 32: I think you want simply "r" for the ARFLAGS. 31, 65: I couldn't find where AUTHDEFS, LIBDEFS, or SFWINCDIR were defined. Are these still appropriate? 65: You shouldn't need to append CPPFLAGS.master. Can you look in a build log and see if these are tacked on twice? 55, 58-59: There are .l, .l.o, .l.po and .l.i suffix rules in usr/src/Makefile.master. You shouldn't need to define these, and the comment on line 55 should actually be wrong. 63-64: Did you consider installing those headers into the proto area during install_h, and then adding them to a packaging exception list? 81: This should be POFILE, instead of MSGDOMAINPOFILE. That's because you're combining all of these together from usr/src/cmd/svr4pkg/Makefile, before installing it into the staging area. The way you're doing it here, the libinst.po messages will be installed into the staging area by themselves, and then ALSO as part of svr4pkg.po. The perfectly acceptable alternative would be to NOT install svr4pkg.po from the parent Makefile, and do all of the subdirs independently, like this. If you want to do that, I can describe how you would need to change the makefiles. 72: This is wrong. You should only pass the install target into this Makefile from ../Makefile if you're doing something with it--it shouldn't be a synonym for "all." Similarly, ../Makefile is passing lint down this way, but you're not handling it. This was my previous comment, about supporting it correctly, even if it's not clean. 74-76: You're not postprocessing the archive but you should be. The build rule should end with $(POST_PROCESS_A). 83: You should be getting the clobber action from usr/src/cmd/Makefile.targ. And "clean" should not remove $(PROG) or $(POFILE), because those are real products of the build. I echo Jim's concerns that mapfile-vers is extraneous, and that scripvfy.c should not be under SCM. Which means that it SHOULD be removed by the clean target. The comment about redefining "clobber" applies to the individual cmd dir makefiles, too. And the comment about only passing down "lint" if you're going to support it. And Jim's comment that "all" should always be the first target in a makefile, so that a "make" with no targets will build "all" instead of (for example) "install." Please review the makefiles and get rid of commented out lines that are leftover from trying to get things working. > ======================================================================== > ======================================================================== > > > 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.
