svn commit: r346341 - head/tools/build

Thu Apr 18 17:37:46 UTC 2019

On April 18, 2019 9:59:50 AM PDT, Warner Losh <imp at bsdimp.com> wrote:
>On Thu, Apr 18, 2019 at 10:26 AM Cy Schubert
><Cy.Schubert at cschubert.com>
>wrote:
>
>> On April 18, 2019 8:11:49 AM PDT, Warner Losh <imp at bsdimp.com> wrote:
>>>
>>>
>>>
>>> On Thu, Apr 18, 2019 at 8:44 AM Cy Schubert
><Cy.Schubert at cschubert.com>
>>> wrote:
>>>
>>>> On April 18, 2019 7:22:53 AM PDT, "Rodney W. Grimes" <
>>>> freebsd at gndrsh.dnsmgr.net> wrote:
>>>> >> On Thu, Apr 18, 2019 at 8:46 AM Rodney W. Grimes
>>>> >> <freebsd at gndrsh.dnsmgr.net> wrote:
>>>> >> >
>>>> >> > > In message <201904180107.x3I17QDc002945 at gndrsh.dnsmgr.net>,
>>>> >"Rodney W.
>>>> >> > > Grimes"
>>>> >> > > writes:
>>>> >> > > > > Author: cy
>>>> >> > > > > Date: Thu Apr 18 01:02:00 2019
>>>> >> > > > > New Revision: 346341
>>>> >> > > > > URL: https://svnweb.freebsd.org/changeset/base/346341
>>>> >> > > > >
>>>> >> > > > > Log:
>>>> >> > > > >   As an interim measure until a more permanent solution
>is
>>>> >implemented
>>>> >> > > > >   workaround the following error:
>>>> >> > > > >
>>>> >> > > > >  
>/usr/src/contrib/elftoolchain/strings/strings.c:198:55:
>>>> >error: use of
>>>> >> > > > >   undeclared identifier
>>>> >> > > > >   'FA_OPEN' fa = fileargs_init(argc, argv, O_RDONLY, 0,
>>>> >&rights, FA_OPEN);
>>>> >> > > > >
>>>> >> > > > >   Reported by:    O. Hartmann <ohartmann at walstatt.org>
>>>> >> > > > >   Reported by:    Michael Butler
><imb at protected-networks.net>
>>>> >> > > > >   Reported by:    gjb@ & cy@ (implicit)
>>>> >> > > > >   Reviewed by:    emaste@
>>>> >> > > > >   Noted by:       rgrimes@
>>>> >> > > > >
>>>> >> > > > > Modified:
>>>> >> > > > >   head/tools/build/Makefile
>>>> >> > > > >
>>>> >> > > > > Modified: head/tools/build/Makefile
>>>> >> > > > >
>>>>
>>>>
>>===========================================================================
>>>> >> > > > ===
>>>> >> > > > > --- head/tools/build/Makefile     Thu Apr 18 00:38:54
>2019
>>>> >    (r34634
>>>> >> > > > 0)
>>>> >> > > > > +++ head/tools/build/Makefile     Thu Apr 18 01:02:00
>2019
>>>> >    (r34634
>>>> >> > > > 1)
>>>> >> > > > > @@ -59,9 +59,7 @@ INCS+=          capsicum_helpers.h
>>>> >> > > > >  INCS+=           libcasper.h
>>>> >> > > > >  .endif
>>>> >> > > > >
>>>> >> > > > > -.if !exists(/usr/include/casper/cap_fileargs.h)
>>>> >> > > > >  CASPERINC+=
>>>> >${SRCTOP}/lib/libcasper/services/cap_fileargs/cap_filea
>>>> >> > > > rgs.h
>>>> >> > > > > -.endif
>>>> >> > > >
>>>> >> > > > As a further note, we should probably hunt for any thing
>>>> >> > > > that is explicity looking at /usr/include/... in a
>Makefile,
>>>> >> > > > as that is minimally missing a ${DESTDIR} argument.
>>>> >> > > >
>>>> >> > > > The above may of actually worked if it had been written:
>>>> >> > > > .if !exists(${DESTDIR}/usr/include/casper/cap_fileargs.h)
>>>> >> > > > someone may wish to test that.
>>>> >> > > >
>>>> >> > > > Also a pathname rooted at / without ${DESTDIR} is almost
>>>> >certainly a mistake.
>>>> >> > >
>>>> >> > > This is a better solution. I tested this in a tree with a
>>>> >duplicated
>>>> >> > > environment: Problem solved. Before this is committed it
>should
>>>> >be
>>>> >> > > tested on one of the universe machines.
>>>> >> >
>>>> >> > From what Ed just said this would also be wrong,
>>>> >> > as well as CASPERINC+= above being wrong, if this
>>>> >> > is being built for the host we should not be using
>>>> >> > any headers from ${SRCTOP} at all.
>>>> >> >
>>>> >> > if capfileargs.h does not exist on the host that functionality
>>>> >> > must not be compiled into the buildtool as the host does not
>>>> >> > have this feature and attempting to use it from SRCTOP is
>wrong.
>>>> >> >
>>>> >>
>>>> >> Keep in mind that this is bootstrap; it's being built for the
>host
>>>> >> system, but it will link against a version of libcasper that's
>been
>>>> >> built in an earlier stage with the proper featureset.
>>>> >
>>>> >Ok, flip flop again, if infact this is linked against a
>>>> >library that implements the stuff from cap_fileargs.h then
>>>> >infact the ${DESTDIR} addition so that the build peaks into
>>>> >the cross build tree is correct, or what ever the equivelent
>>>> >to DESTDIR is for that ?  BUILDDIR?  The point is it should
>>>> >be picking this header up from the object tree, NOT from
>>>> >the running system.
>>>>
>>>> Yes, this was my conclusion when working on kerberos and ntp. This
>is
>>>> also true of libraries,  else one would need to installworld and
>buildworld
>>>> again to get a properly built library/binary.
>>>>
>>>> IIRC ngie@ fixed a number of these across the tree a couple of
>years
>>>> ago.
>>>
>>>
>>> OK. There's a number of different issues going on. As the original
>author
>>> of libegacy (which is what we're seeing fail), let me address the
>design
>>> generically and comment on different things that have come up in
>this
>>> thread.
>>>
>>> Since this is going into the libegacy that we're using to build the
>>> system, the check for file is bogus, for reasons I'll discuss below.
> When
>>> we add new includes to the system, it is appropriate to do it this
>way. And
>>> when this file was added to the system, the check was correct.
>>>
>>> First off, DESTDIR is absolutely not correct since this is to build
>the
>>> legacy library and legacy includes which augment the host's sources
>on
>>> legacay system, hence the name. It's never the correct thing to use.
>>>
>>> The problem that we have here is not that the file is missing (which
>is
>>> why it was added the way it was a long time ago), but rather missing
>>> functionality in a file that's been around for a long time.
>>>
>>> So, since it is that class of problem, the canonical way we've dealt
>with
>>> it in the past is to do something like create a file foo.h that
>looks
>>> something like
>>>
>>> #include_next <foo.h>
>>>
>>> #ifndev SOMETHING_NEW
>>> #define SOMETHING_NEW 0 /* Or other values that are fail-safe on the
>old
>>> system */
>>> #endif
>>>
>>> and that's the change that needs to be made here. Sometimes, more
>>> extensive things need to be done when the old library can't work at
>all
>>> with the new code. In those cases, the pattern is for foo.h to
>include
>>> #define problem_fn my_problem_fn and then write a my_problem_fn that
>wraps
>>> problem_fn in a way that works on the old system. Kinda a poor man's
>symbol
>>> versioning, in a way (note: we can't use symbol version for this
>since
>>> we're building new binaries).
>>>
>>> The "stop gap" gets things compiling, and maybe OK for the moment,
>unless
>>> the SOMETHING_NEW variable that's used (in this case FA_OPEN) causes
>the
>>> old library to do the wrong thing. I've not done the deep dive to
>see if
>>> this is the case or not.
>>>
>>> So, does that make sense?
>>>
>>> Warner
>>>
>>
>> This solves one problem but what about the cases when a new krb5,
>ntp, or
>> amd is imported but fails to build because it is using the old
>headers in
>> /usr/include and linking against old libraries on the running system?
>>
>> These examples BTW have been fixed. My concern is there could be
>other
>> examples in contrib, yet to be discovered, that might also have the
>same
>> issues.
>>
>
>That's the whole point if libegacy: to provide the glue that's needed
>for
>the bootstrap process. I'm surprised that ntp or amd are involved with
>libegacy at all since they aren't bootstrap tools. Are you sure you
>aren't
>confusing two different problem domains? I know kerberos is, but it
>kinda
>needs to be.
>
>Warner

Maybe I am conflating the two issues. I'll look again the next time I get to a keyboard.

-- 
Pardon the typos and autocorrect, small keyboard in use.
Cheers,
Cy Schubert <Cy.Schubert at cschubert.com>
FreeBSD UNIX: <cy at FreeBSD.org> Web: http://www.FreeBSD.org

	The need of the many outweighs the greed of the few.