From owner-svn-src-all@FreeBSD.ORG Thu Jan 19 01:54:58 2012 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E1ED6106566B; Thu, 19 Jan 2012 01:54:58 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail09.syd.optusnet.com.au (mail09.syd.optusnet.com.au [211.29.132.190]) by mx1.freebsd.org (Postfix) with ESMTP id 7D3FD8FC1E; Thu, 19 Jan 2012 01:54:58 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q0J1ssdf028834 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 19 Jan 2012 12:54:56 +1100 Date: Thu, 19 Jan 2012 12:54:54 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Peter Wemm In-Reply-To: <201201181826.q0IIQuLX051824@svn.freebsd.org> Message-ID: <20120119110628.F1702@besplex.bde.org> References: <201201181826.q0IIQuLX051824@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r230311 - head/lib/libpam/modules/pam_unix X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Jan 2012 01:54:59 -0000 On Wed, 18 Jan 2012, Peter Wemm wrote: > Log: > Rev 228065 (change bsd.own.mk -> bsd.init.mk) broke pam_unix.so by causing > the LDADD/DPADD to lose the -lpam, and causing openpam_dynamic() to fail > due to "openpam_get_options" being undefined. bsd.init.mk (actually this abuse of it -- see below) is horribily broken. It includes ../Makefile.inc, so anything that includes it early tends to get ../Makefile.inc in a wrong order. The normal order is to include bsd.prog.mk late, and get ../Makefile.inc late from there. Any early include of a bsd.*.mk file is dangerous because it is not the normal order. Unfortunately, a few Makefiles need to include bsd.own.mk early. bsd.init.mk includes bsd.own.mk, so these Makefiles can now (actually for a long time) include bsd.init.mk instead. I think bsd.init.mk is an implementation detail and including it directly is just bug. Its documentation still agrees with this interpretatation: - the comment at the start of it only says that it is used at the top of all files. - the automatic include of ../Makefile.inc is only documented (in bsd.README) for , and . BTW, why the bracket quoting for these? - bsd.README documents bsd.init.mk as being for the make include files. In /usr/src on Nov 13 2011, the only Makefile[.inc] files with this bug are: gnu/usr.bin/cc/include/Makefile lib/clang/include/Makefile sys/boot/toomany/Makefile (this matches the general low quality of boot Makefiles) The bug in libpam was too new to show up here. > > This would cause obscure console log messages like: > openpam_dynamic(): No error: 0 > openpam_load_module(): no pam_unix.so found > and other helpful messages which are no help in diagnosing the problem. > > Fortunately this change was not mfc'ed to 9.x, it isn't broken there. > > Modified: > head/lib/libpam/modules/pam_unix/Makefile > > Modified: head/lib/libpam/modules/pam_unix/Makefile > ============================================================================== > --- head/lib/libpam/modules/pam_unix/Makefile Wed Jan 18 18:22:25 2012 (r230310) > +++ head/lib/libpam/modules/pam_unix/Makefile Wed Jan 18 18:26:56 2012 (r230311) > @@ -40,8 +40,8 @@ LIB= pam_unix > SRCS= pam_unix.c > MAN= pam_unix.8 > > -DPADD= ${LIBUTIL} ${LIBCRYPT} > -LDADD= -lutil -lcrypt > +DPADD+= ${LIBUTIL} ${LIBCRYPT} > +LDADD+= -lutil -lcrypt Bitrot near here started in 2004 when the tabs were lost. > > .if ${MK_NIS} != "no" Including bsd.own.mk is unfortunately necessary for getting MK_NIS defined (bsd.own.mk is abused to define MK_* and compatibility cruft that are both completely unrelated to ownership). That is enough for the "few" Makefiles that test MK_*. (More than 200 Makefiles include bsd.own.mk explicitly, presumably only (early) for testimg MK_* :-(.) According to the log message for the commit that broke this, bsd.init.mk is used (early) instead of bsd.own.mk (early) to allow use of settings from ../Makefile.inc. This seems to be actually to use settings in the user's make.conf or environment or defaults to kill CTF in an old implementation of killing CTF. Now, bsd.own.mk is abused further to kill CTF, and I can't see why it doesn't do this when only it is included early. > CFLAGS+= -DYP > I don't like the structure of this file or tree at all. The library order is important for static linkage, so the implementation detail that bsd.init.mk includes ../Makefile.inc is important (it only does this for the benefit of bsd.prog.mk etc. which are documented to do it. According to a strict reading of the documentation, only bsd.prog.mk does it, so the order is wrong here). Actually, ../Makefile.inc has always added -lpam, but before this included bsd.init.mk, its order was precisely the opposite of what it is now -- it was last instead of first. It's not clear how either order can work. -lpam first seems right (in case this library refers to symbols in the others), but the old order seemed to work. I checked all the other includes of bsd.init.mk. All except the boot Makefiles use it correctly (near the end, for the technical reason that they include something like bsd.obj.mk and not bsd.prog.mk, so they don't include it in the normal way via bsd.prog.mk). The boot makefiles document that they are using it to get ../Makefile.inc. They should just include ../Makefile.inc for that. They need it mainly to get the BTX* symbols defined. Related bugs: - sys.mk was polluted in 2004 with an unconditional include of bsd.compat.mk, just after the place where previous FreeBSD-build- specific includes are ifdefed to reduce such pollution. - the include of bsd.compat.mk in bsd.init.mk has no effect, since sys.mk has already done it unconditionally. - MK_CTF is still used unitialized kern.post.mk and kern.pre.mk. It is uninitialized when the kernel source tree is current and the host mk non-kernel files more than a couple of months old. The corresponding bug in kmod.mk was only there for a few days. Bruce