Moriah Waterland writes: > 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.
This isn't intended as the right long-term answer, is it? We've tried to avoid static libraries, even for internal use. It seems to me that this should be factored into a normal shared library. (Yes, I realize that things like the weird quit() function need some minor redesign to work properly.) > http://cr.opensolaris.org/~mwaterl/6739234.changes_only/webrev/ General comments: The "/*LINTLIBRARY*/" token doesn't belong in any .c file. If you see one there, and you're editing the file anyway, please remove it. Make sure you do a "make clobber" build and then check your workspace with 'hg status'. I think some of the clobber targets will remove things that 'hg' knows about -- either because of bad clobber targets or because of things checked into 'hg' that shouldn't be there. Reviewed, no comments: usr/src/cmd/Makefile usr/src/cmd/svr4pkg/hdrs/libadm.h usr/src/cmd/svr4pkg/hdrs/messages.h usr/src/cmd/svr4pkg/installf/dofinal.c usr/src/cmd/svr4pkg/installf/installf.c usr/src/cmd/svr4pkg/installf/installf.h usr/src/cmd/svr4pkg/libinst/copyf.c usr/src/cmd/svr4pkg/libinst/cvtpath.c usr/src/cmd/svr4pkg/libinst/depchk.c usr/src/cmd/svr4pkg/libinst/dockdeps.c usr/src/cmd/svr4pkg/libinst/doulimit.c usr/src/cmd/svr4pkg/libinst/dryrun.c usr/src/cmd/svr4pkg/libinst/dryrun.h usr/src/cmd/svr4pkg/libinst/echo.c usr/src/cmd/svr4pkg/libinst/eptstat.c usr/src/cmd/svr4pkg/libinst/finalck.c usr/src/cmd/svr4pkg/libinst/findscripts.c usr/src/cmd/svr4pkg/libinst/flex_dev.c usr/src/cmd/svr4pkg/libinst/install.h usr/src/cmd/svr4pkg/libinst/is_local_host.c usr/src/cmd/svr4pkg/libinst/libinst.h usr/src/cmd/svr4pkg/libinst/listmgr.c usr/src/cmd/svr4pkg/libinst/lockinst.c usr/src/cmd/svr4pkg/libinst/log.c usr/src/cmd/svr4pkg/libinst/mntinfo.c usr/src/cmd/svr4pkg/libinst/nblk.c usr/src/cmd/svr4pkg/libinst/ocfile.c usr/src/cmd/svr4pkg/libinst/open_package_datastream.c usr/src/cmd/svr4pkg/libinst/pathdup.c usr/src/cmd/svr4pkg/libinst/pkgobjmap.c usr/src/cmd/svr4pkg/libinst/pkgops.c usr/src/cmd/svr4pkg/libinst/procmap.c usr/src/cmd/svr4pkg/libinst/psvr4ck.c usr/src/cmd/svr4pkg/libinst/ptext.c usr/src/cmd/svr4pkg/libinst/putparam.c usr/src/cmd/svr4pkg/libinst/qreason.c usr/src/cmd/svr4pkg/libinst/qstrdup.c usr/src/cmd/svr4pkg/libinst/scriptvfy.l usr/src/cmd/svr4pkg/libinst/setadmin.c usr/src/cmd/svr4pkg/libinst/setlist.c usr/src/cmd/svr4pkg/libinst/setup_temporary_directory.c usr/src/cmd/svr4pkg/libinst/sml.c usr/src/cmd/svr4pkg/libinst/srcpath.c usr/src/cmd/svr4pkg/libinst/stub.c usr/src/cmd/svr4pkg/libinst/unpack_package_from_stream.c usr/src/cmd/svr4pkg/pkgadd/msgdefs.h usr/src/cmd/svr4pkg/pkgadm/addcert.c usr/src/cmd/svr4pkg/pkgadm/certs.c usr/src/cmd/svr4pkg/pkgadm/listcert.c usr/src/cmd/svr4pkg/pkgadm/lock.c usr/src/cmd/svr4pkg/pkgadm/pkgadm.h usr/src/cmd/svr4pkg/pkgadm/pkgadm_msgs.h usr/src/cmd/svr4pkg/pkgadm/removecert.c usr/src/cmd/svr4pkg/pkgchk/checkmap.c usr/src/cmd/svr4pkg/pkgchk/ckentry.c usr/src/cmd/svr4pkg/pkgcond/main.c usr/src/cmd/svr4pkg/pkgcond/pkgcond.h usr/src/cmd/svr4pkg/pkgcond/pkgcond_msgs.h usr/src/cmd/svr4pkg/pkginstall/backup.c usr/src/cmd/svr4pkg/pkginstall/check.c usr/src/cmd/svr4pkg/pkginstall/cppath.c usr/src/cmd/svr4pkg/pkginstall/dockspace.c usr/src/cmd/svr4pkg/pkginstall/getinst.c usr/src/cmd/svr4pkg/pkginstall/instvol.c usr/src/cmd/svr4pkg/pkginstall/merginfo.c usr/src/cmd/svr4pkg/pkginstall/msgdefs.h usr/src/cmd/svr4pkg/pkginstall/pkgenv.c usr/src/cmd/svr4pkg/pkginstall/pkginstall.h usr/src/cmd/svr4pkg/pkginstall/pkgvolume.c usr/src/cmd/svr4pkg/pkginstall/predepend.c usr/src/cmd/svr4pkg/pkginstall/quit.c usr/src/cmd/svr4pkg/pkginstall/reqexec.c usr/src/cmd/svr4pkg/pkginstall/sortmap.c usr/src/cmd/svr4pkg/pkgmk/mkpkgmap.c usr/src/cmd/svr4pkg/pkgmk/quit.c usr/src/cmd/svr4pkg/pkgmk/splpkgmap.c usr/src/cmd/svr4pkg/pkgparam/pkgparam.c usr/src/cmd/svr4pkg/pkgremove/check.c usr/src/cmd/svr4pkg/pkgremove/delmap.c usr/src/cmd/svr4pkg/pkgremove/predepend.c usr/src/cmd/svr4pkg/pkgremove/quit.c usr/src/cmd/svr4pkg/pkgremove/special.c usr/src/cmd/svr4pkg/pkgremove/wsreg_pkgrm.c usr/src/cmd/svr4pkg/pkgremove/wsreg_pkgrm.h usr/src/cmd/svr4pkg/pkgrm/check.c usr/src/cmd/svr4pkg/pkgrm/main.c usr/src/cmd/svr4pkg/pkgrm/msgdefs.h usr/src/cmd/svr4pkg/pkgrm/presvr4.c usr/src/cmd/svr4pkg/pkgrm/quit.c usr/src/cmd/svr4pkg/pkgrm/quit.h usr/src/cmd/svr4pkg/pkgscripts/default usr/src/cmd/svr4pkg/pkgscripts/i.CompCpio usr/src/cmd/svr4pkg/pkgscripts/i.awk usr/src/cmd/svr4pkg/pkgscripts/i.build usr/src/cmd/svr4pkg/pkgscripts/i.sed usr/src/cmd/svr4pkg/pkgscripts/r.awk usr/src/cmd/svr4pkg/pkgscripts/r.build usr/src/cmd/svr4pkg/pkgscripts/r.sed usr/src/cmd/svr4pkg/pkgscripts/special_contents usr/src/lib/libinstzones/Makefile usr/src/lib/libinstzones/common/zones_args.c usr/src/lib/libinstzones/common/zones_exec.c usr/src/lib/libinstzones/common/zones_locks.c usr/src/lib/libinstzones/common/zones_lofs.c usr/src/lib/libinstzones/common/zones_paths.c usr/src/lib/libinstzones/common/zones_states.c usr/src/lib/libinstzones/common/zones_str.c usr/src/lib/libinstzones/common/zones_strings.h usr/src/lib/libinstzones/sparc/Makefile usr/src/lib/libpkg/Makefile usr/src/lib/libpkg/common/canonize.c usr/src/lib/libpkg/common/cfext.h usr/src/lib/libpkg/common/cvtpath.c usr/src/lib/libpkg/common/dbtables.h usr/src/lib/libpkg/common/devtype.c usr/src/lib/libpkg/common/dstream.c usr/src/lib/libpkg/common/fmkdir.c usr/src/lib/libpkg/common/gpkglist.c usr/src/lib/libpkg/common/gpkgmap.c usr/src/lib/libpkg/common/handlelocalfs.c usr/src/lib/libpkg/common/isdir.c usr/src/lib/libpkg/common/keystore.c usr/src/lib/libpkg/common/keystore.h usr/src/lib/libpkg/common/llib-lpkg usr/src/lib/libpkg/common/logerr.c usr/src/lib/libpkg/common/mapfile-vers usr/src/lib/libpkg/common/mappath.c usr/src/lib/libpkg/common/ncgrpw.c usr/src/lib/libpkg/common/nhash.c usr/src/lib/libpkg/common/pkgerr.c usr/src/lib/libpkg/common/pkgerr.h usr/src/lib/libpkg/common/pkgexecl.c usr/src/lib/libpkg/common/pkgexecv.c usr/src/lib/libpkg/common/pkglibmsgs.h usr/src/lib/libpkg/common/pkglocale.h usr/src/lib/libpkg/common/pkgmount.c usr/src/lib/libpkg/common/pkgstr.c usr/src/lib/libpkg/common/pkgtrans.c usr/src/lib/libpkg/common/pkgweb.h usr/src/lib/libpkg/common/pkgxpand.c usr/src/lib/libpkg/common/ppkgmap.c usr/src/lib/libpkg/common/putcfile.c usr/src/lib/libpkg/common/rrmdir.c usr/src/lib/libpkg/common/runcmd.c usr/src/lib/libpkg/common/security.c usr/src/lib/libpkg/common/srchcfile.c usr/src/lib/libpkg/common/tputcfent.c usr/src/lib/libpkg/common/verify.c usr/src/lib/libpkg/common/vfpops.c usr/src/lib/libpkg/sparc/Makefile usr/src/pkgdefs/Makefile usr/src/pkgdefs/SUNWpkgcmdsr/Makefile usr/src/pkgdefs/SUNWpkgcmdsr/pkginfo.tmpl usr/src/pkgdefs/SUNWpkgcmdsr/prototype_i386 usr/src/pkgdefs/SUNWpkgcmdsr/prototype_sparc usr/src/pkgdefs/SUNWpkgcmdsu/Makefile usr/src/pkgdefs/SUNWpkgcmdsu/depend usr/src/pkgdefs/SUNWpkgcmdsu/pkginfo.tmpl usr/src/pkgdefs/SUNWpkgcmdsu/prototype_i386 usr/src/pkgdefs/SUNWpkgcmdsu/prototype_sparc Comments: usr/src/cmd/Makefile.cmd 62,140,144: this already exists; suggest using existing $(ROOTBIN) and related symbols instead. 144,145: duplicated line? 328: stray "XXX" 329-330: this isn't right; it expands as: $(SCRIPT:%=$(ROOTUSRSADM)/%)/%: % ... which means that you're addressing targets that are subdirectories of the members of the $(SCRIPT) variable, and no such thing is delivered. usr/src/cmd/svr4pkg/Makefile 37: suggest alphabetic order. 71-73,91,93-94: this shouldn't be needed; add new directories to Targetdirs instead. 87,90: stray; please remove. 88: clean, clobber, and lint targets do not need .WAIT; only "all" does. 91: this doesn't look right to me; is "install" supposed to build "_msg"? Doesn't this result in building "_msg" twice? (Could eliminate special install target and just fold into same for 'all'.) 97-98: $(SUBDIRS) is never set. 103-104: stray XXX comment; please remove. 104: nit: spelling error 105,108: nit: merge as: $(LIBSUBDIR) $(CMDSUBDIRS): FRC @cd $@; pwd; $(MAKE) $(TARGET) 111: why are the dependencies for clean and clobber defined again here? (Why not define them here and remove them entirely from line 88?) 114-115: unnecessary; creates duplicate targets with duplicate rules. usr/src/cmd/svr4pkg/hdrs/install.h Not sure why this is a new file. usr/src/cmd/svr4pkg/hdrs/libinst.h Not sure why new. 134,139: ew. 155-159: double ew. This header file definitely needs a spring cleaning. Not this project, though. usr/src/cmd/svr4pkg/installf/Makefile 44: should be $(ROOTBIN) 52: nit: '-lc' likely isn't needed here. 60: nit: 'all' target should be first (before 'install' on line 58). Otherwise, a bare "make" will cause it to install. 75: is this needed? usr/src/cmd/svr4pkg/installf/main.c 23: time went backwards (old lines) 417,508: these look like accidentally removed bug fixes. usr/src/cmd/svr4pkg/installf/removef.c 107: accidentally removed fix? usr/src/cmd/svr4pkg/libinst/Makefile 55: misplaced comment (actually refers to line 58, and was moved?). 84: needed? usr/src/cmd/svr4pkg/libinst/fixpath.c 822: looks like a removed bug fix. usr/src/cmd/svr4pkg/libinst/isreloc.c 48: definitely inappropriate comment for ON. 54: remove. usr/src/cmd/svr4pkg/libinst/mapfile-vers What is this file doing ... ? You're not building a shared library, and the file doesn't have the required "MAPFILE HEADER" text nor any usable symbols. usr/src/cmd/svr4pkg/libinst/pkgdbmerg.c 23: time rolled backwards 196: removed bug fix. usr/src/cmd/svr4pkg/libinst/scriptvfy.c This file should *not* be part of your workspace. It's a derived object (generated by running lex on scriptvfy.l). It needs to be removed with "hg rm". usr/src/cmd/svr4pkg/pkgadd/Makefile 51: '-lc' likely not needed. 54,75: stray; remove. 61: 'all' target should be first. 68: needed? usr/src/cmd/svr4pkg/pkgadd/check.c 46-49: looks like an accidentally reverted bug fix. (The path for this should include hdrs, so <> should work.) usr/src/cmd/svr4pkg/pkgadd/main.c 76-79: looks like an accidentally reverted bug fix. (The path for this should include hdrs, so <> should work.) usr/src/cmd/svr4pkg/pkgadd/presvr4.c 60-63: ditto usr/src/cmd/svr4pkg/pkgadd/quit.c 41-43: ditto 265-267: remove 268: nit: excess parenthesis usr/src/cmd/svr4pkg/pkgadd/quit.h 43: ditto usr/src/cmd/svr4pkg/pkgadm/Makefile 48-51,53-55: lots of duplicated stuff across makefiles; consider creating "usr/src/cmd/svr4pkg/Makefile.com" to hold it. 53: '-lc' 62: 'all' target should be first. 74: needed? usr/src/cmd/svr4pkg/pkgadm/main.c 56-58,61-63: stray; remove 59: why was this made a global symbol? usr/src/cmd/svr4pkg/pkgchk/Makefile 50: '-lc' 59: 'all' 71: needed? usr/src/cmd/svr4pkg/pkgchk/main.c 536: removed (but wrongish) previous bug fix. usr/src/cmd/svr4pkg/pkgcond/Makefile 48: '-lc' 48: linkage to libzonecfg needs to be lazy, or this will fail on systems without Zones installed. 49: possible issues with these other libraries; need to check packaging. 69: needed? usr/src/cmd/svr4pkg/pkginfo/Makefile 48: '-lc' 56: 'all' usr/src/cmd/svr4pkg/pkginfo/pkginfo.c 321,323: reverted (and still wrong) bug fix. usr/src/cmd/svr4pkg/pkginstall/Makefile 65: '-lc' 73: 'all' 88: needed? usr/src/cmd/svr4pkg/pkginstall/main.c 2244,2246: bug fix usr/src/cmd/svr4pkg/pkgmk/Makefile usual comments usr/src/cmd/svr4pkg/pkgmk/main.c 691: reverted bug fix usr/src/cmd/svr4pkg/pkgname/Makefile usual comments usr/src/cmd/svr4pkg/pkgname/pkgname.c 53,55: bug fix usr/src/cmd/svr4pkg/pkgparam/Makefile usr/src/cmd/svr4pkg/pkgproto/Makefile usual comments (on both) usr/src/cmd/svr4pkg/pkgproto/main.c 165,167: bug fix usr/src/cmd/svr4pkg/pkgremove/Makefile usual comments usr/src/cmd/svr4pkg/pkgremove/main.c 1125,1127: bug fix usr/src/cmd/svr4pkg/pkgremove/wsreg_pkgrm_test.c Is this file even used? It doesn't seem to be, and could just be eliminated from the workspace. usr/src/cmd/svr4pkg/pkgrm/Makefile usual comments usr/src/cmd/svr4pkg/pkgscripts/Makefile 52-55,58-59: this swilly little 'cmdexec' program doesn't seem to use most of this. 61-66: doesn't belong here. usr/src/cmd/svr4pkg/pkgscripts/cmdexec.c 126,128: bug fix usr/src/cmd/svr4pkg/pkgtrans/Makefile usual comments usr/src/cmd/svr4pkg/pkgtrans/main.c 211: bug usr/src/lib/Makefile 96,98: this doesn't look like the right place for these things. The libraries after the .WAIT on line 102 do _not_ depend implicitly on these two libraries. usr/src/lib/libinstzones/Makefile.com 58: this should probably be a lazy load for libzonecfg. usr/src/lib/libinstzones/common/instzones_api.h 62-69: suggest removing comment blocks that you're not actively using. 75: nit: format string should be const, and should have /* PRINTFLIKE1 */ here. usr/src/lib/libinstzones/common/instzones_lib.h 62-65: delete 305,307,309,344,363,366: fmt should be const. 377: use ANSI '(void)'. usr/src/lib/libinstzones/common/llib-linstzones It's unclear to me what the distinction is between instzones_api.h and instzones_lib.h. I thought instzones_lib.h was something internal to the construction of the library, and not used by things that link to the library. If that's correct, then it should *NOT* be included here. Only things that consumers of the library use belong in the llib-l* file. If instzones_lib.h is actually used by the consumers, then maybe the API is ill-defined. 33: this is almost certainly not right. There's nothing here for lint to consume. usr/src/lib/libinstzones/common/mapfile-vers Missing "MAPFILE HEADER" block. Should cause 'nightly' build errors. 25: stray usr/src/lib/libinstzones/common/zones.c 23: lost a year? (Several files here are similar.) usr/src/lib/libinstzones/common/zones_utils.c 125,127: why are errors printed to stdout instead of stderr? 125: how does "%s" help here? (Looks like a concern about translation attacks that I think were fixed long ago, but not sure ...) 639,653,665,677: nit: correspondence between comments and code is a little arbitrary. usr/src/lib/libinstzones/i386/Makefile 28: missing $(ROOTLINT)? (Have you built nightly on x86?) usr/src/lib/libpkg/Makefile.com 55: stray usr/src/lib/libpkg/common/ckparam.c 31: as long as you're here, this line is wrong and needs to be removed. usr/src/lib/libpkg/common/ckvolseq.c 31: ditto usr/src/lib/libpkg/common/p12lib.c usr/src/lib/libpkg/common/p12lib.h Please seek out legal help with this part. I don't think that putting this code (which clearly belongs to someone else) under CDDL is the right thing to do. The original Sun copyright notice (without CDDL) was probably correct. Plus, you'll need to update the third party license files. usr/src/lib/libpkg/common/pkglib.h 614: as long as you're adding a function, giving it a proper prototype would be nice. usr/src/lib/libpkg/common/pkgweb.c 1831-1832: what is this? usr/src/lib/libpkg/common/progerr.c 31: misplaced. 48: errors on stdout doesn't seem right. usr/src/lib/libpkg/i386/Makefile 28: stray usr/src/pkgdefs/SUNWpkgcmdsr/depend This doesn't look like a significant 'depend' file. Is it needed? usr/src/pkgdefs/SUNWpkgcmdsr/prototype_com 34-36: assignments shouldn't be neede here. usr/src/pkgdefs/SUNWpkgcmdsu/prototype_com 33,34: stray 80: we typically don't ship compilation symlinks for internal libraries. Will anything outside of ON compile against this? -- James Carlson, Solaris Networking <james.d.carlson at sun.com> Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084 MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
