Bug 110211 - If DESTDIR is set to an empty string, the dri drivers are not installed
Summary: If DESTDIR is set to an empty string, the dri drivers are not installed
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Other (show other bugs)
Version: 19.0
Hardware: All All
: medium normal
Assignee: Dylan Baker
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-03-20 15:26 UTC by Pierre Labastie
Modified: 2019-03-22 19:09 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
attachment-9884-0.html (1.93 KB, text/html)
2019-03-21 12:58 UTC, Dylan Baker
Details
fix DESTDIR handling in install_megadrivers.py (856 bytes, patch)
2019-03-21 13:29 UTC, Benoit Pierre
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre Labastie 2019-03-20 15:26:43 UTC
Dear maintainer,

The script "bin/install_megadrivers.py" contains the following:
---
if os.path.isabs(args.libdir):
        to = os.path.join(os.environ.get('DESTDIR', '/'), args.libdir[1:])
    else:
        to = os.path.join(os.environ['MESON_INSTALL_DESTDIR_PREFIX'], args.libdir)
---
The issue is when libdir is absolute, and DESTDIR is set to the empty string. This is very likely to occur in build scripts of the form (many of us at http://www.linuxfromscratch.org do that):
---
#PKG_DEST=/some/where
...
DESTDIR=$PKG_DEST ninja install
---
where we comment out the PKG_DEST assignment or not, depending on whether we want a DESTDIR install or not.

Then the "to" variable becomes a relative path, and the drivers are not installed. This is not really what is expected, I guess. I'd suggest using (but I am far from being a Python geek):
---
if os.path.isabs(args.libdir):
        to = os.path.join(os.environ.get('DESTDIR', ''), args.libdir)
---
That worked for me in all cases I have tried (may not have thought of all of them...)
Comment 1 Dylan Baker 2019-03-20 17:51:48 UTC
I never expected anyone to set DESTDIR to '', so this is definitely a bug.

At one point meson didn't enforce that libdir had to be under prefix, and os.path.join will ignore any arguments that precede an absolute path. I'll send a patch to fix this.
Comment 2 Dylan Baker 2019-03-20 22:00:51 UTC
This should be fixed in master, and the patch will be in the next 19.0 and 18.3 releases.
Comment 3 Pierre Labastie 2019-03-21 10:28:16 UTC
Thanks for the quick fix.
Comment 4 Rafael Castillo 2019-03-21 11:34:06 UTC
Hi, dylan your patch broke ArchLinux PKGBUILD for mesa since your patch for some reason bypass fakeinstall paths and try to install directly in /usr/lib, any suggestions?
Comment 5 Dylan Baker 2019-03-21 12:58:20 UTC
Created attachment 143746 [details]
attachment-9884-0.html

:(

Yes, I know how to fix it. Just need more code.

I'm on mobile, please excuse autocorrect fail.

On Thu, Mar 21, 2019, 04:34 <bugzilla-daemon@freedesktop.org> wrote:

> *Comment # 4 <https://bugs.freedesktop.org/show_bug.cgi?id=110211#c4> on
> bug 110211 <https://bugs.freedesktop.org/show_bug.cgi?id=110211> from
> Rafael Castillo <jrch2k10@gmail.com> *
>
> Hi, dylan your patch broke ArchLinux PKGBUILD for mesa since your patch for
> some reason bypass fakeinstall paths and try to install directly in /usr/lib,
> any suggestions?
>
> ------------------------------
> You are receiving this mail because:
>
>    - You are the assignee for the bug.
>
>
Comment 6 Benoit Pierre 2019-03-21 13:29:54 UTC
Created attachment 143747 [details] [review]
fix DESTDIR handling in install_megadrivers.py

This how I fixed it locally. I only tested the PKGBUILD on Arch Linux (so with DESTDIR set). I tried to ensure Windows is correctly supported to (with the correct drive being used in all cases).
Comment 7 Denis 2019-03-21 14:23:17 UTC
agree, also faced with this issue. Reproduced it on Ubuntu 18.04 and Manjaro.
Comment 8 Pierre Labastie 2019-03-21 15:41:17 UTC
(In reply to Benoit Pierre from comment #6)
> Created attachment 143747 [details] [review] [review]
> fix DESTDIR handling in install_megadrivers.py
> 
> This how I fixed it locally. I only tested the PKGBUILD on Arch Linux (so
> with DESTDIR set). I tried to ensure Windows is correctly supported to (with
> the correct drive being used in all cases).

Looks like your patch comes back to the original issue:
This is Ok if DESTDIR is not set, or if DESTDIR is set to a non empty string,
but not if DESTDIR is set to an empty string. Note that this is because os.path.join is not consistent:
a slash is inserted when doing os.path.join('a','b'), but not when doing
os.path.joint('','b')!

Note that the fix I proposed is bad (sorry): 
if you do, for instance, "to = os.path.join('/sources/install','/usr/lib/dri')" in python, then "to" is set to '/usr/lib/dri'... My bad (I thought I had tried).

Maybe something like (I'm not sure what to do with win32):
---
if os.path.isabs(args.libdir):
    destdir=os.environ.get('DESTDIR','')
    if len(destdir) == 0:
        destdir = '/'
    to = os.path.join(destdir, args.libdir[1:])
  else:
...
Comment 9 Dylan Baker 2019-03-21 16:09:14 UTC
---
if os.path.isabs(args.libdir):
    destdir=os.environ.get('DESTDIR','')
    if len(destdir) == 0:
        destdir = '/'
    to = os.path.join(destdir, args.libdir[1:])
  else:
...

Don't use len there, len() for testing emptiness is a python antipattern :)

just `if not destdir:`

However, I think doing something like

destdir = os.environ.get('DESTDIR')
to = args.libdir
if destdir
    to = os.path.join(destdir, to)

is probably sufficient.

Just FYI, we don't do patch review in bugzilla, if you'd like to propose a patch either an MR at gitlab.freedesktop.org/mesa/mesa or a patch sent to mesa-dev@lists.freedesktop.org is the proper way.
Comment 10 Luke A. Guest 2019-03-21 16:48:43 UTC
(In reply to Dylan Baker from comment #9)
> ---
> if os.path.isabs(args.libdir):
>     destdir=os.environ.get('DESTDIR','')
>     if len(destdir) == 0:
>         destdir = '/'
>     to = os.path.join(destdir, args.libdir[1:])
>   else:
> ...

This fixes it for me.
Comment 11 Dylan Baker 2019-03-21 18:11:03 UTC
Have a look at this MR, which is basically the same patch with some style cleanups:

https://gitlab.freedesktop.org/mesa/mesa/merge_requests/505
Comment 12 Pierre Labastie 2019-03-21 20:39:11 UTC
I told you I was not a Python geek :) The MR seems to work in all cases I can imagine (unset DESTDIR, empty DESTDIR, DESTDIR set to non-empty) when abs.libdir starts with '/' (sorry if this is not the place to comment on the MR). Thanks for the Python lesson, it's my first day with Python (almost) ;)
Comment 13 Benoit Pierre 2019-03-22 11:53:18 UTC
Commenting on an issue does not add you to the CC list? How stupid is that...
Comment 14 Dylan Baker 2019-03-22 19:09:41 UTC
Should be fixed in master now


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.