Re: [gentoo-dev] [PATCH] systemd.eclass: remove SYSROOT from pkg-config output

[James Le Cuirot <chewi@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 4486 bytes --]

On Sun, 3 Jan 2021 10:16:49 -0500
Mike Gilbert <floppym@gentoo.org> wrote:

> On Sun, Jan 3, 2021 at 7:52 AM James Le Cuirot <chewi@gentoo.org> wrote:
> >
> > On Sat,  2 Jan 2021 20:09:04 -0500
> > Mike Gilbert <floppym@gentoo.org> wrote:
> >  
> > > When cross-compiling, users will typically have
> > > PKG_CONFIG_SYSROOT=${SYSROOT} defined via pkg-config wrapper.
> > >
> > > When PKG_CONFIG_SYSROOT is set, all paths included in pkg-config
> > > output get prefixed with this value.
> > >
> > > Signed-off-by: Mike Gilbert <floppym@gentoo.org>
> > > ---
> > >
> > > This patch has already been pushed, but I figured I would send it for
> > > review in case someone else can think of a failure case, or has a better
> > > solution.
> > >
> > >  eclass/systemd.eclass | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/eclass/systemd.eclass b/eclass/systemd.eclass
> > > index 81065a0af79a..f6d1fa2d92d6 100644
> > > --- a/eclass/systemd.eclass
> > > +++ b/eclass/systemd.eclass
> > > @@ -50,6 +50,7 @@ _systemd_get_dir() {
> > >
> > >       if $(tc-getPKG_CONFIG) --exists systemd; then
> > >               d=$($(tc-getPKG_CONFIG) --variable="${variable}" systemd) || die
> > > +             d=${d#${SYSROOT}}
> > >               d=${d#${EPREFIX}}
> > >       else
> > >               d=${fallback}  
> 
> > The EPREFIX line is (sometimes) wrong in EAPI 7 though and the same goes
> > for udev.eclass. It took a while to get this agreed and corrected in
> > PMS but if SYSROOT points to / then the effective prefix is BROOT, not
> > EPREFIX; they may not be the same.  
> 
> Ugh, that "corrected" logic in PMS head is nasty.

I wish it could be simpler but that's just the nature of the beast. I
could give an example of why it matters, even in this context, but I
think you already know. There's some good news though...

> > If you just strip ESYSROOT then
> > it will always do the right thing but you'll need this fall back for
> > older EAPIs. I'm not sure why you didn't do it in one line?  
> 
> I was trying to accommodate older EAPIs that do not define ESYSROOT.
> This seemed like the simplest approach. It also allows for the case
> where someone is not using cross-pkg-config, and
> PKG_CONFIG_SYSROOT_DIR is unset. Since SYSROOT and EPREFIX are removed
> separately, if one of them is already missing, it will just be
> skipped.

I saw your new patch. I noticed that it sets the "d" variable but
doesn't make use of it. That aside, I thought perhaps I could come up
with something cleaner. That's when I made an amusing discovery.

$ PKG_CONFIG_SYSROOT_DIR=/foo pkg-config --variable=udevdir udev      
/lib/udev

The udevdir variable is not affected by PKG_CONFIG_SYSROOT_DIR at all.
And why would it be? The man page says that this variable is only
applied to -I and -L flags. I don't know for sure but I suspect that
pkg-config just sees this as some arbitrary variable with no special
path handling at all. I wonder what led you to think that this fix was
necessary?

That said, you'll still need special handling for EAPI 7. Unfortunately
there's no separate variable for the prefix alone but at least it's
simpler now. Something like this?

diff --git a/eclass/udev.eclass b/eclass/udev.eclass
index 2873ae9a92c3..f10ea0de01a2 100644
--- a/eclass/udev.eclass
+++ b/eclass/udev.eclass
@@ -50,12 +50,19 @@ fi
 # @DESCRIPTION:
 # Get unprefixed udevdir.
 _udev_get_udevdir() {
-       if $($(tc-getPKG_CONFIG) --exists udev); then
-               local udevdir="$($(tc-getPKG_CONFIG) --variable=udevdir udev)"
-               echo "${udevdir#${EPREFIX%/}}"
-       else
-               echo /lib/udev
+       local udevdir="/lib/udev"
+
+       if $(tc-getPKG_CONFIG) --exists udev; then
+               udevdir="$($(tc-getPKG_CONFIG) --variable=udevdir udev)" || die
+
+               if [[ ${EAPI:-0} == [0123456] ]]; then
+                       udevdir=${udevdir#${EPREFIX}}
+               else
+                       udevdir=${udevdir#${ESYSROOT#${SYSROOT}}}
+               fi
        fi
+
+       echo "${udevdir}"
 }

One last question. Why is this dynamic at all? Shouldn't it just be
hardcoded to /lib/udev? Sure, a user could patch udev to make it
something different if they really wanted but there are plenty of other
paths we just assume. What makes this one special?

-- 
James Le Cuirot (chewi)
Gentoo Linux Developer

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]