svn commit: r366962 - in head: include usr.bin/calendar

[Alex Kozlov]

On Mon, Oct 26, 2020 at 12:11:56AM -0600, Warner Losh wrote:
> On Mon, Oct 26, 2020 at 12:01 AM Alex Kozlov <ak at freebsd.org> 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