From owner-svn-src-all@freebsd.org Tue Oct 27 07:37:54 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id B5B9D4411EF; Tue, 27 Oct 2020 07:37:54 +0000 (UTC) (envelope-from kozlov@ravenloft.kiev.ua) Received: from ravenloft.kiev.ua (ravenloft.kiev.ua [94.244.131.95]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4CL3TF4jCVz4RbT; Tue, 27 Oct 2020 07:37:53 +0000 (UTC) (envelope-from kozlov@ravenloft.kiev.ua) Date: Tue, 27 Oct 2020 08:37:44 +0100 From: Alex Kozlov To: Warner Losh Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r366962 - in head: include usr.bin/calendar Message-ID: <20201027073744.GA27357@ravenloft.kiev.ua> References: <202010230922.09N9MNZu040921@repo.freebsd.org> <20201024074840.GA26119@ravenloft.kiev.ua> <38d15142-1cb1-eb1f-215e-cee165743d99@freebsd.org> <20201025055633.GA52119@ravenloft.kiev.ua> <0140ae63-3044-9946-4047-c64331be0b50@freebsd.org> <20201026060038.GA78455@ravenloft.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Queue-Id: 4CL3TF4jCVz4RbT X-Spamd-Bar: / Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=none (mx1.freebsd.org: domain of kozlov@ravenloft.kiev.ua has no SPF policy when checking 94.244.131.95) smtp.mailfrom=kozlov@ravenloft.kiev.ua X-Spamd-Result: default: False [-0.02 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-0.67)[-0.673]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; TO_DN_SOME(0.00)[]; NEURAL_HAM_LONG(-0.03)[-0.026]; MIME_GOOD(-0.10)[text/plain]; DMARC_NA(0.00)[FreeBSD.org]; AUTH_NA(1.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; NEURAL_HAM_SHORT(-0.52)[-0.518]; R_SPF_NA(0.00)[no SPF record]; FORGED_SENDER(0.30)[ak@FreeBSD.org,kozlov@ravenloft.kiev.ua]; RCVD_COUNT_ZERO(0.00)[0]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:34743, ipnet:94.244.128.0/18, country:UA]; FROM_NEQ_ENVFROM(0.00)[ak@FreeBSD.org,kozlov@ravenloft.kiev.ua]; MAILMAN_DEST(0.00)[svn-src-all,svn-src-head] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.33 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: Tue, 27 Oct 2020 07:37:54 -0000 On Mon, Oct 26, 2020 at 12:11:56AM -0600, Warner Losh wrote: > On Mon, Oct 26, 2020 at 12:01 AM Alex Kozlov wrote: > > > On Sun, Oct 25, 2020 at 11:37:34AM +0100, Stefan Esser wrote: > > > Am 25.10.20 um 06:56 schrieb Alex Kozlov: > > > > On Sat, Oct 24, 2020 at 04:37:45PM +0200, Stefan Esser wrote: > > > > > Am 24.10.20 um 09:48 schrieb Alex Kozlov: > > > [...] > > > > > > You are hardcoding assumption that LOCALBASE = /usr/local. Please > > make it > > > > > > overridable with LOCALBASE environment variable. > > > > > This was a trivial change to get us going with calendars provided by > > > > > a port (which has not been committed, yet - therefore there are no > > > > > port-provided calendars, neither under /usr/local nor under any other > > > > > PREFIX, as of now). > > > > > > > > > I understand what you are asking for, but in such a case I'd rather > > > > > think you want to rebuild FreeBSD with _PATH_LOCALBASE modified in > > > > > paths.h. > > > > The PREFIX != LOCALBASE and both != /usr/local configurations > > > > are supported in the ports tree and the base for a long time, please > > see > > > > > > https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/porting-prefix.html > > > > > > Yes, and I do not need to look that up in the handbook, having been > > > a ports committer for 2 decades by now. > > > > > > > If after this commit you need to rebuild base to use non-default > > LOCALBASE/PREFIX > > > > it is pretty big regression and POLA. > > > > > > How is that any different than before? > > > > > > What I did is make the PATH easier to change when you rebuild base. > > > > > > There are numerous programs in base that contain the literal string > > > /usr/local - and what I did was implement a mechanism that allows > > > to replace this literal reference with a simple change in paths.h. > > > > > > If you do not modify paths.h for a different LOCALBASE, then you'll > > > get a wrong _PATH_DEFPATH compiled into your binaries, for example. > > > > > > > > And I have made this a single instance that needs to be changed. > > > > > Before my change there were 2 instances of /usr/local hard-coded > > > > > in _PATH_DEFPATH - now you have to only change the definition of > > > > > _PATH_LOCALBASE to adjust all 3 locations that use it. > > > > I think you made situation worse, there were two stray hardcoded > > > > string and now there is official LOCALBASE define which likely will be > > > > used by other people in the future. > > > > > > I'd hope so to get rid of many of the 1713 literal uses of /usr/local > > > in our source tree. > > > > > > > > If you can show me precedence of a LOCALBASE environment variable > > > > > being used in the way you suggest, I'd be willing to make calendar > > > > > use it. > > > > Just an analogy from LOCALBASE make variable, perhaps CALENDAR_HOME > > > > is a better name. > > > > > > Yes, I already suggested CALENDAR_HOME, but as an environment variable > > > to check, if you want to be able to path an additional directory (or > > > search path) to the calendar program at run-time. But why introduce > > > a CALENDAR_HOME macro in the sources, if the port supplied calendar > > > files are known to be found at LOCALBASE/share/calendar (for some value > > > of LOCALBASE). > > > > > > I want to make more programs that currently hard-code /usr/local use > > > _PATH_LOCALBASE instead. This C macro can then be default to /usr/local > > > but can be overridden by passing LOCALBASE to the compiler (from the > > > build infrastructure) when paths.h is included. > > > > > > Instead of referring to _PATH_LOCALBASE these files could directly use > > > LOCALBASE, but since other paths are defined as _PATH_xxx in paths.h I > > > think it is best to follow this precedent. > > > > > > > > But then I think a CALENDAR_HOME variable would be even more useful, > > > > > since it would allow to search an additional user selected directory > > > > > (and not just share/calendar within what you provide as LOCALBASE). > > > > > > My change did not add any dependency on LOCALBASE to any previously > > > existing functionality. It added support for calendar files provided > > > by a port (a feature that did not exist before) at a location that is > > > correct for the big majority of users (who do not modify LOCALBASE). > > > > > > As I said: I'm going to make it easier to build the base system with > > > a different LOCALBASE, but not by run-time checking an environment > > > variable that specifies LOCALBASE in each affected program. > > It seems that you intend to follow through no matter what. So, just for > > the record, I think that hardcoding LOCALBASE and requiring base rebuild > > to change it is a very wrong approach. > > > > So, first off, it's already hard coded. Stefan's changes change the hard > coding from 'impossible to change' to 'changeable with a recompile' which > is an improvement. It might even wind up as a build variable (or not, doing > that has some really ugly, nasty dependencies). > > But even in ports-land, it's a compile time constant. Quite a large number > of ports will allow you to change it at compile / build time, but not > after. You have to rebuild if you want to change PREFIX... > > So I'm a bit puzzled what makes this the wrong approach? 1) Making it buildtime instead of fixing a few regression cases which as simple as reading environment variable before fallback to hardcoded /usr/local, or make it kernel variable/sysctl if security is a concern. 2) Codifying LOCALBASE = /usr/local, so from now more people will use it because it's in defines. -- Alex