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().
CANTFIX. This would break unpack ${A}.
What's the problem? We don't need it anymore since users have 'default' instead.
If we're looking to fix unpack, perhaps we should introduce a new saner alternative with a different name.
(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.
(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.
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. ;-)
(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
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).
(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}.
(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".
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
(In reply to Ulrich Müller from comment #11) i'd be OK with that
(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?
(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.
In EAPI 6: https://gitweb.gentoo.org/proj/pms.git/commit/?id=217e63be1592ae258ac4761c634414036252e60c