Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 483244 - [Future EAPI] unpack() should accept absolute paths
Summary: [Future EAPI] unpack() should accept absolute paths
Status: RESOLVED FIXED
Alias: None
Product: Gentoo Hosted Projects
Classification: Unclassified
Component: PMS/EAPI (show other bugs)
Hardware: All Linux
: Normal enhancement (vote)
Assignee: PMS/EAPI
URL:
Whiteboard: in-eapi-6
Keywords:
Depends on:
Blocks: future-eapi
  Show dependency tree
 
Reported: 2013-09-01 06:56 UTC by Michał Górny
Modified: 2015-11-08 21:52 UTC (History)
1 user (show)

See Also:
Package list:
Runtime testing required: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2013-09-01 06:56:18 UTC
All arguments to unpack must be either a filename without path,
  in which case unpack looks in DISTDIR for the file, or start with
  the string ./, in which case unpack uses the argument as a path
  relative to the working directory.

This is fairly incompatible with whatever most of the helpers and utilities do. Since 'unpack' is used rarely, let's make it take a normal path like:

1. "${DISTDIR}"/foo.tar -> as said,

2. foo.tar -> from current directory.

Please also make it accept full paths for bug #334275, so we wouldn't need to do anything really hacky.

Note that this will likely require rewriting default src_unpack().
Comment 1 Ulrich Müller gentoo-dev 2013-09-01 07:04:04 UTC
CANTFIX. This would break unpack ${A}.
Comment 2 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2013-09-01 07:04:56 UTC
What's the problem? We don't need it anymore since users have 'default' instead.
Comment 3 Ciaran McCreesh 2013-09-01 07:10:45 UTC
If we're looking to fix unpack, perhaps we should introduce a new saner alternative with a different name.
Comment 4 Ulrich Müller gentoo-dev 2013-09-01 07:18:55 UTC
(In reply to Michał Górny from comment #2)
> What's the problem? We don't need it anymore since users have 'default'
> instead.

It introduces confusing EAPI dependent behaviour, for very little benefit, and breaks a common syntax that was working for > 10 years.

You really want unpack to accept random absolute paths, so that people will use it for things in ${FILESDIR} and whatnot?


(In reply to Ciaran McCreesh from comment #3)
> If we're looking to fix unpack, perhaps we should introduce a new saner
> alternative with a different name.

Yes, _if_ we are going to fix it.
Comment 5 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2013-09-01 09:02:26 UTC
(In reply to Ulrich Müller from comment #4)
> (In reply to Michał Górny from comment #2)
> > What's the problem? We don't need it anymore since users have 'default'
> > instead.
> 
> It introduces confusing EAPI dependent behaviour, for very little benefit,
> and breaks a common syntax that was working for > 10 years.

Cargo cult here? The problem is that the syntax is *confusing* by design. Most of the developers have even no idea of the magic inside it. I, for one, just assumed 'unpack ${A}' assumed ${A} having absolute paths.

The function has confusing conditional behavior already. Adding EAPI condition won't make it much worse, while in a few years it will finally allow us to get rid of the burden of confusing syntax introduced >10 years ago.

> You really want unpack to accept random absolute paths, so that people will
> use it for things in ${FILESDIR} and whatnot?

I was rather thinking of files outside repository structure (in local repos) and less confusing syntax for unpacking files inside cwd and DISTDIR.
Comment 6 Ulrich Müller gentoo-dev 2013-09-01 10:06:33 UTC
The common case for unpack is to operate on something in DISTDIR. In very few cases (like Debian packages) some double unpacking is needed and unpack must operate on a relative path. The current implementation is well suited for this. There's no point in cluttering ebuilds with 'unpack "${DISTDIR}"/foo.tar.xz' or 'local x; for x in ${A}; do unpack ${x##*/}; done' where currently a simple 'unpack foo.tar.xz' or 'unpack ${A}' does the job.

As much as I'm elsewhere for consistency, the unpack function is special here. It doesn't buy us anything to change its default to operate on files in the cwd. Typically, the cwd (i.e. WORKDIR) is empty when the function is called.

From bug 334275 comment 8 I conclude that until yesterday you weren't even aware that unpack cannot operate on absolute paths. That's not a good base for arguing that this would be urgently needed functionality. ;-)
Comment 7 Ulrich Müller gentoo-dev 2013-09-01 10:08:27 UTC
(In reply to Ulrich Müller from comment #6)
> for x in ${A}; do unpack ${x##*/}; done

Other way around: for x in ${A}; do unpack "${DISTDIR}"/${x}; done
Comment 8 SpanKY gentoo-dev 2013-09-03 02:20:37 UTC
i wouldn't mind updating the code to accept paths that start with / or ../, but it's pretty rare that that is actually needed.  off the top of my head, it comes up when you have an unpacked archive that you then want to unpack somewhere else:
src_unpack() {
    unpack foo.tar.gz # has bar.tar inside of it
    cd "${S}"/blah
    unpack "${S}"/bar.tar
}

it can be worked around in a multiple step process:
src_unpack() {
    unpack foo.tar.gz # has bar.tar inside of it
    cd "${S}"/blah
    ln -s "${S}"/bar.tar
    unpack ./bar.tar
    rm bar.tar
}

but that doesn't mean i think unpack should be changed to require absolute paths if you want stuff out of $DISTDIR, nor should $A get populated with absolute paths (white space issues could only be fixed by also changing $A to an array).
Comment 9 Ulrich Müller gentoo-dev 2013-09-03 06:09:49 UTC
(In reply to SpanKY from comment #8)
> i wouldn't mind updating the code to accept paths that start with / or ../,
> but it's pretty rare that that is actually needed.

It was explicitly requested in bug 201771 that unpack shouldn't accept absolute paths. And my impression is that this functionality isn't missed.

> [...]

> but that doesn't mean i think unpack should be changed to require absolute
> paths if you want stuff out of $DISTDIR, nor should $A get populated with
> absolute paths (white space issues could only be fixed by also changing $A
> to an array).

+1

Prefixing the path with ${DISTDIR} is just the most convenient thing to do in src_unpack. Similarly, many src_install helpers prefix the path with ${ED}.
Comment 10 SpanKY gentoo-dev 2013-09-04 05:46:48 UTC
(In reply to Ulrich Müller from comment #9)

i think the idea in bug 201771 was that the error message was straight up confusing (it'd tell you the arg it was passed instead of the arg it tried to actually unpack).  from there, we just moved on to clarifying the code rather than having a discussion about "should absolute paths be accepted in general".
Comment 11 Ulrich Müller gentoo-dev 2013-09-04 06:07:02 UTC
Then how about this?
- if the argument is a filename without path (i.e. contains no slash), then
  prefix it with ${DISTDIR}/
- otherwise take the literal path
Comment 12 SpanKY gentoo-dev 2013-09-04 18:45:58 UTC
(In reply to Ulrich Müller from comment #11)

i'd be OK with that
Comment 13 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2014-08-18 08:19:44 UTC
(In reply to Ulrich Müller from comment #11)
> Then how about this?
> - if the argument is a filename without path (i.e. contains no slash), then
>   prefix it with ${DISTDIR}/
> - otherwise take the literal path

Is this the final behavior for EAPI6?
Comment 14 Ulrich Müller gentoo-dev 2014-08-19 13:27:13 UTC
(In reply to Michał Górny from comment #13)
> (In reply to Ulrich Müller from comment #11)
> > Then how about this?
> > - if the argument is a filename without path (i.e. contains no slash), then
> >   prefix it with ${DISTDIR}/
> > - otherwise take the literal path
> 
> Is this the final behavior for EAPI6?

Yes.