Github tarballs (was: localpatch() fails if $S initially has a bogus value)
Jonas Bernoulli
> I believe the problem is because of how we are handling github's tarballs.
There is a problem in how we are handling github's tarballs (or rather
there is a
problem with github's tarballs).
However it is not actually related with the localpatch issue. It's
just that in the case I
did describe the two issues were effective at the same time so I
mentioned them in
the same mail. But let's forget about the localpatch issue in this thread.
> As example, dev-ruby/thor is doing:
>
> SRC_URI="http://github.com/wycats/${PN}/tarball/v${PV} ->
> ${PN}-git-${PV}.tgz"
> S="${WORKDIR}/wycats-${PN}-*"
I don't know why this actually works - probably some magic hidden in an eclass.
> To the fix is portage's ebuilds and something in localpatch to expend $S
> before use. Propably something like:
> S="$(eval echo $S)" just before running patch in localpatch.
Huh? The easiest and ugliest solution is to simply cd to
${WORKDIR}/${GITHUB_USER}-${PN}-*
at the beginning of every function that would normally be runs with PWD=$S.
But that is quite ugly. Daniel is doing something like this e.g. in
sys-cluster/vzctl:
GITHUB_USER="funtoo"
GITHUB_TAG="${PF}-funtoo"
SRC_URI="https://github.com/${GITHUB_USER}/${PN}/tarball/${GITHUB_TAG}
-> ${GITHUB_TAG}.tar.gz"
src_prepare() {
cd "${WORKDIR}"/${GITHUB_USER}-${PN}-*
S="$(pwd)"
}
This is an okay solution, mostly (see below).
We would want something like the below but it won't work:
GITHUB_USER="funtoo"
GITHUB_TAG="${PF}-funtoo"
SRC_URI="https://github.com/${GITHUB_USER}/${PN}/tarball/${GITHUB_TAG}
-> ${GITHUB_TAG}.tar.gz"
S="${WORKDIR}"/${GITHUB_USER}-${PN}-*
Why doesn't it work? Try this:
$ cd /tmp
$ touch foo
$ var=fo*
$ echo $var
foo
$ rm foo
$ echo $var
fo*
The * is not actually expanded until the variable is used. Using
$(eval echo ...) won't change
anything as the directory we are trying to expand to does not exist
yet when we do it at the
ebuild top-level.
The next best choice is to set S as soon as it can be expanded:
src_unpack() {
unpack ${A}
S="${WORKDIR}"/${GITHUB_USER}-${PN}-*
}
This is pretty much what Daniel is doing with the only exception that
it is done a little bit earlier.
Just so much earlier that it works in combination with the localpatch feature.
But this actually is something that should be done in in src_prepare
not src_unpack, but then
it just does not work if we also want to apply user patches before src_prepare.
So I guess we either have to
(a) live with this ugliness,
(b) convince the github devs to name the top-level directory based on
the tag or simply repo name
(rather than the sha, the filename could still be named based on that), or
(c) implement ->> similar to -> just that it not only renames the
tarball but also the top-level
directory within.
(d) don't use these tarballs
-- Jonas
Piotr Karbowski
That whas the idea to make localpatch work with $S what contain * since
it running patch with -d "${S}" so it will put there *, eval will expend
it, not really related to ebuild itself.
> But this actually is something that should be done in in src_prepare
> not src_unpack, but then
> it just does not work if we also want to apply user patches before src_prepare.
There is no good way to handle it, if patch should be added before or
after src_prepare. When I was designing localpatch I was even thinking
about put it into src_unpack.
> So I guess we either have to
>
> (a) live with this ugliness,
> (b) convince the github devs to name the top-level directory based on
> the tag or simply repo name
> (rather than the sha, the filename could still be named based on that), or
> (c) implement ->> similar to -> just that it not only renames the
> tarball but also the top-level
> directory within.
> (d) don't use these tarballs
as a ugly workaround, I could put some code to check:
if $S exist, if not, check if $WORKDIR contain one dir, if yes, then set
is as new $S (local S= so it will affect only localpatch function). But
I am not sure if putting such workaround is good idea.
-- Piotr.
Jonas Bernoulli
> On 01/31/2011 01:00 AM, Jonas Bernoulli wrote:
>> The * is not actually expanded until the variable is used. Using
>> $(eval echo ...) won't change
>> anything as the directory we are trying to expand to does not exist
>> yet when we do it at the
>> ebuild top-level.
>
> That whas the idea to make localpatch work with $S what contain * since it
> running patch with -d "${S}" so it will put there *, eval will expend it,
> not really related to ebuild itself.
I misunderstood you; thought you were saying this should be done in ebuilds.
In the localpatch code it would of course work since the directory already
exists.
>> But this actually is something that should be done in in src_prepare
>> not src_unpack, but then
>> it just does not work if we also want to apply user patches before
>> src_prepare.
>
> There is no good way to handle it, if patch should be added before or after
> src_prepare. When I was designing localpatch I was even thinking about put
> it into src_unpack.
what about
/etc/portage/patches/before/...
/etc/portage/patches/after/...
:-/
> as a ugly workaround, I could put some code to check:
> if $S exist, if not, check if $WORKDIR contain one dir, if yes, then set is
> as new $S (local S= so it will affect only localpatch function). But I am
> not sure if putting such workaround is good idea.
Just do be sure: this is about localpatch not ebuilds, right?
Even if you do this to enable github+localpatch then ebuild authors would still
have to set S in src_unpack or src_prepare.
Me to tired for this right now.
-- Jonas
Piotr Karbowski
> what about
> /etc/portage/patches/before/...
> /etc/portage/patches/after/...
>
> :-/
No idea.
> Just do be sure: this is about localpatch not ebuilds, right?
Of course.
> Even if you do this to enable github+localpatch then ebuild authors would still
> have to set S in src_unpack or src_prepare.
To support github's tarballs? Propably. To make localpatch working? Not
really. If $S is not fixed before localpatch, then:
if [ ! -d "${S}" ]; then
[ "$(find $WORKDIR -depth 1 -type d | wc -l)" = 1 ] && S=$WORKDIR/*
fi
(I presume that $WORKDIR is for example
/var/tmp/portage/app-cat/foo-1.0-r1/workdir/)
This is super ugly example but should proove the idea.
-- Piotr.
Jonas Bernoulli
>> Even if you do this to enable github+localpatch then ebuild authors would
>> still
>> have to set S in src_unpack or src_prepare.
>
> To support github's tarballs? Propably. To make localpatch working? Not
> really. If $S is not fixed before localpatch, then:
>
> if [ ! -d "${S}" ]; then
> [ "$(find $WORKDIR -depth 1 -type d | wc -l)" = 1 ] && S=$WORKDIR/*
> fi
>
> (I presume that $WORKDIR is for example
> /var/tmp/portage/app-cat/foo-1.0-r1/workdir/)
>
> This is super ugly example but should proove the idea.
If there are other cases but the github problem where $S is not fixed
before localpatch is called then I think this should be done.
-- Jonas
Piotr Karbowski
> If there are other cases but the github problem where $S is not fixed
> before localpatch is called then I think this should be done.
I think there is none. In general messing with $S is not good idea.
There is a bug related to this
http://bugs.gentoo.org/show_bug.cgi?id=350478 "Allow changing S from
ebuild phases"
I believe that instead of fixing $S better would be rename github's dir
name to proper $S, like I did in fusecompress:
## code
github_user="tex"
github_tag="${PV}"
SRC_URI="https://github.com/${github_user}/${PN}/tarball/${github_tag}
-> ${PN}-git-${PV}.tgz"
src_unpack() {
unpack ${A}
mv "${WORKDIR}/${github_user}-${PN}"-??????? "${S}" || die
}
## code
I think this is good time to write about known limitations of localpatch
to wiki.
-- Piotr.