[PATCH 4/4] makepkg: Fixed sourceball creation of split packages

[Eric Bélanger-2]

In some case, the install script of split packages was not included in the sourceballs. This was due because split packages can specify their install script in the package functions.  They also can use several install scripts.

Signed-off-by: Eric Bélanger <snowmaniscool@...>
---
 scripts/makepkg.sh.in |   25 ++++++++++++++++++-------
 1 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index 8fc7c82..b77cc68 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -1060,14 +1060,25 @@ create_srcpackage() {
  msg2 "$(gettext "Adding %s...")" "$BUILDSCRIPT"
  ln -s "${BUILDFILE}" "${srclinks}/${pkgbase}/${BUILDSCRIPT}"
 
- if [ -n "$install" ]; then
- if [ -f $install ]; then
- msg2 "$(gettext "Adding install script...")"
- ln -s "${startdir}/$install" "${srclinks}/${pkgbase}/"
- else
- error "$(gettext "Install scriptlet (%s) does not exist.")" "$install"
+ local install_files
+ install_files=$(grep "install=" "$BUILDSCRIPT")
+ for pkg in ${pkgname[@]}; do
+ install_files+=' '
+ install_files+=$(echo $install_files |sed "s/\$pkgname/$pkg/"|sed "s/\${pkgname}/$pkg/")
+ install_files=$(eval echo $install_files |tr '[:blank:]' '\n'|sort |uniq)
+ done
+
+ for f in $install_files; do
+ install="${f#"install="}"
+ if [[ -n "$install" ]]; then
+ if [[ -f $install ]]; then
+ msg2 "$(gettext "Adding install script...")"
+ ln -s "${startdir}/$install" "${srclinks}/${pkgbase}/"
+ else
+ error "$(gettext "Install scriptlet (%s) does not exist.")" "$install"
+ fi
  fi
- fi
+ done
 
  if [ -n "$changelog" ]; then
  if [ -f "$changelog" ]; then
--
1.6.5.2

[Allan McRae-3]

Eric Bélanger wrote:
> In some case, the install script of split packages was not included in the sourceballs. This was due because split packages can specify their install script in the package functions.  They also can use several install scripts.
>

Please add newlines to the commit message to keep them ~80 characters wide.

Note that this will need done for changelogs too as they are now handled
like install files.

> Signed-off-by: Eric Bélanger <snowmaniscool@...>
> ---
>  scripts/makepkg.sh.in |   25 ++++++++++++++++++-------
>  1 files changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 8fc7c82..b77cc68 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -1060,14 +1060,25 @@ create_srcpackage() {
>   msg2 "$(gettext "Adding %s...")" "$BUILDSCRIPT"
>   ln -s "${BUILDFILE}" "${srclinks}/${pkgbase}/${BUILDSCRIPT}"
>  
> - if [ -n "$install" ]; then
> - if [ -f $install ]; then
> - msg2 "$(gettext "Adding install script...")"
> - ln -s "${startdir}/$install" "${srclinks}/${pkgbase}/"
> - else
> - error "$(gettext "Install scriptlet (%s) does not exist.")" "$install"
> + local install_files
> + install_files=$(grep "install=" "$BUILDSCRIPT")
> + for pkg in ${pkgname[@]}; do
> + install_files+=' '
> + install_files+=$(echo $install_files |sed "s/\$pkgname/$pkg/"|sed "s/\${pkgname}/$pkg/")
> + install_files=$(eval echo $install_files |tr '[:blank:]' '\n'|sort |uniq)
> + done

This is too inclusive.  If I have one package with an install file
$pkgname.install, then it will add an install file $pkg.install for
every $pkg in $pkgname.  Some of these may not exist which will lead to
trouble below when the files presence is tested for.

I do not know how to fix that issue... Two potential options are:
1) add install/changelog files to the source array
2) change the split PKGBUILD format to allow easier evaluation of
variables within split packages.
Both have their drawbacks.

> +
> + for f in $install_files; do
> + install="${f#"install="}"
> + if [[ -n "$install" ]]; then
> + if [[ -f $install ]]; then
> + msg2 "$(gettext "Adding install script...")"
> + ln -s "${startdir}/$install" "${srclinks}/${pkgbase}/"
> + else
> + error "$(gettext "Install scriptlet (%s) does not exist.")" "$install"
> + fi
>   fi
> - fi
> + done
>  
>   if [ -n "$changelog" ]; then
>   if [ -f "$changelog" ]; then

[Cedric Staniewski]

Eric Bélanger wrote:
> In some case, the install script of split packages was not included in the sourceballs. This was due because split packages can specify their install script in the package functions.  They also can use several install scripts.

I'm not sure if I ever tested srcpkg creation with a split PKGBUILD, but it is very likely that this also applies to changelog files given that they are implemented the same way as install files.

[Eric Bélanger-2]

On Fri, Nov 6, 2009 at 10:36 AM, Allan McRae <allan@...> wrote:
> Eric Bélanger wrote:
>>
>> In some case, the install script of split packages was not included in the
>> sourceballs. This was due because split packages can specify their install
>> script in the package functions.  They also can use several install scripts.
>>
>
> Please add newlines to the commit message to keep them ~80 characters wide.

OK.  I'll try to remember next time.

>
> Note that this will need done for changelogs too as they are now handled
> like install files.

ChangeLog are not  included in sourceball   even for non split
package: $changelog doesn't get defined.  This will need to be fixed.
We can  test if there is a ChangeLog file in $startdir and if so we
include it. I could make a patch.

>
>> Signed-off-by: Eric Bélanger <snowmaniscool@...>
>> ---
>>  scripts/makepkg.sh.in |   25 ++++++++++++++++++-------
>>  1 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
>> index 8fc7c82..b77cc68 100644
>> --- a/scripts/makepkg.sh.in
>> +++ b/scripts/makepkg.sh.in
>> @@ -1060,14 +1060,25 @@ create_srcpackage() {
>>        msg2 "$(gettext "Adding %s...")" "$BUILDSCRIPT"
>>        ln -s "${BUILDFILE}" "${srclinks}/${pkgbase}/${BUILDSCRIPT}"
>>  -       if [ -n "$install" ]; then
>> -               if [ -f $install ]; then
>> -                       msg2 "$(gettext "Adding install script...")"
>> -                       ln -s "${startdir}/$install"
>> "${srclinks}/${pkgbase}/"
>> -               else
>> -                       error "$(gettext "Install scriptlet (%s) does not
>> exist.")" "$install"
>> +       local install_files
>> +       install_files=$(grep "install=" "$BUILDSCRIPT")
>> +       for pkg in ${pkgname[@]}; do
>> +               install_files+=' '
>> +               install_files+=$(echo $install_files |sed
>> "s/\$pkgname/$pkg/"|sed "s/\${pkgname}/$pkg/")
>> +               install_files=$(eval echo $install_files |tr '[:blank:]'
>> '\n'|sort |uniq)
>> +       done
>
> This is too inclusive.  If I have one package with an install file
> $pkgname.install, then it will add an install file $pkg.install for every
> $pkg in $pkgname.  Some of these may not exist which will lead to trouble
> below when the files presence is tested for.

I copied this from makechrootpkg.  I just tested it when a package in
a split package doesn't use the install script and it works.

>
> I do not know how to fix that issue... Two potential options are:
> 1) add install/changelog files to the source array
> 2) change the split PKGBUILD format to allow easier evaluation of variables
> within split packages.
> Both have their drawbacks.
>
>> +
>> +       for f in $install_files; do
>> +               install="${f#"install="}"
>> +               if [[ -n "$install" ]]; then
>> +                       if [[ -f $install ]]; then
>> +                               msg2 "$(gettext "Adding install
>> script...")"
>> +                               ln -s "${startdir}/$install"
>> "${srclinks}/${pkgbase}/"
>> +                       else
>> +                               error "$(gettext "Install scriptlet (%s)
>> does not exist.")" "$install"
>> +                       fi
>>                fi
>> -       fi
>> +       done
>>          if [ -n "$changelog" ]; then
>>                if [ -f "$changelog" ]; then
>
>
>

[Eric Bélanger-2]

On Fri, Nov 6, 2009 at 10:59 AM, Eric Bélanger <snowmaniscool@...> wrote:

> On Fri, Nov 6, 2009 at 10:36 AM, Allan McRae <allan@...> wrote:
>> Eric Bélanger wrote:
>>>
>>> In some case, the install script of split packages was not included in the
>>> sourceballs. This was due because split packages can specify their install
>>> script in the package functions.  They also can use several install scripts.
>>>
>>
>> Please add newlines to the commit message to keep them ~80 characters wide.
>
> OK.  I'll try to remember next time.
>
>>
>> Note that this will need done for changelogs too as they are now handled
>> like install files.
>
> ChangeLog are not  included in sourceball   even for non split
> package: $changelog doesn't get defined.  This will need to be fixed.
> We can  test if there is a ChangeLog file in $startdir and if so we
> include it. I could make a patch.

Forget about that. As Cedric mentionned earlier and Xavier explained
on IRC, ChangeLog will be handled like install files. I just got
confused for a moment.

>
>>
>>> Signed-off-by: Eric Bélanger <snowmaniscool@...>
>>> ---
>>>  scripts/makepkg.sh.in |   25 ++++++++++++++++++-------
>>>  1 files changed, 18 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
>>> index 8fc7c82..b77cc68 100644
>>> --- a/scripts/makepkg.sh.in
>>> +++ b/scripts/makepkg.sh.in
>>> @@ -1060,14 +1060,25 @@ create_srcpackage() {
>>>        msg2 "$(gettext "Adding %s...")" "$BUILDSCRIPT"
>>>        ln -s "${BUILDFILE}" "${srclinks}/${pkgbase}/${BUILDSCRIPT}"
>>>  -       if [ -n "$install" ]; then
>>> -               if [ -f $install ]; then
>>> -                       msg2 "$(gettext "Adding install script...")"
>>> -                       ln -s "${startdir}/$install"
>>> "${srclinks}/${pkgbase}/"
>>> -               else
>>> -                       error "$(gettext "Install scriptlet (%s) does not
>>> exist.")" "$install"
>>> +       local install_files
>>> +       install_files=$(grep "install=" "$BUILDSCRIPT")
>>> +       for pkg in ${pkgname[@]}; do
>>> +               install_files+=' '
>>> +               install_files+=$(echo $install_files |sed
>>> "s/\$pkgname/$pkg/"|sed "s/\${pkgname}/$pkg/")
>>> +               install_files=$(eval echo $install_files |tr '[:blank:]'
>>> '\n'|sort |uniq)
>>> +       done
>>
>> This is too inclusive.  If I have one package with an install file
>> $pkgname.install, then it will add an install file $pkg.install for every
>> $pkg in $pkgname.  Some of these may not exist which will lead to trouble
>> below when the files presence is tested for.
>
> I copied this from makechrootpkg.  I just tested it when a package in
> a split package doesn't use the install script and it works.
>
>>
>> I do not know how to fix that issue... Two potential options are:
>> 1) add install/changelog files to the source array
>> 2) change the split PKGBUILD format to allow easier evaluation of variables
>> within split packages.
>> Both have their drawbacks.
>>
>>> +
>>> +       for f in $install_files; do
>>> +               install="${f#"install="}"
>>> +               if [[ -n "$install" ]]; then
>>> +                       if [[ -f $install ]]; then
>>> +                               msg2 "$(gettext "Adding install
>>> script...")"
>>> +                               ln -s "${startdir}/$install"
>>> "${srclinks}/${pkgbase}/"
>>> +                       else
>>> +                               error "$(gettext "Install scriptlet (%s)
>>> does not exist.")" "$install"
>>> +                       fi
>>>                fi
>>> -       fi
>>> +       done
>>>          if [ -n "$changelog" ]; then
>>>                if [ -f "$changelog" ]; then
>>
>>
>>
>

[Allan McRae-3]

Eric Bélanger wrote:

> On Fri, Nov 6, 2009 at 10:36 AM, Allan McRae <allan@...> wrote:
>> Eric Bélanger wrote:
>>> In some case, the install script of split packages was not included in the
>>> sourceballs. This was due because split packages can specify their install
>>> script in the package functions.  They also can use several install scripts.
>>>
>> Please add newlines to the commit message to keep them ~80 characters wide.
>
> OK.  I'll try to remember next time.
>
>> Note that this will need done for changelogs too as they are now handled
>> like install files.
>
> ChangeLog are not  included in sourceball   even for non split
> package: $changelog doesn't get defined.  This will need to be fixed.
> We can  test if there is a ChangeLog file in $startdir and if so we
> include it. I could make a patch.
>

On the master branch, you are required to add "changelog=<file>" to
include a changelog in your package.  That was added to allow split
packages to have different changelogs in different packages.

>>> Signed-off-by: Eric Bélanger <snowmaniscool@...>
>>> ---
>>>  scripts/makepkg.sh.in |   25 ++++++++++++++++++-------
>>>  1 files changed, 18 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
>>> index 8fc7c82..b77cc68 100644
>>> --- a/scripts/makepkg.sh.in
>>> +++ b/scripts/makepkg.sh.in
>>> @@ -1060,14 +1060,25 @@ create_srcpackage() {
>>>        msg2 "$(gettext "Adding %s...")" "$BUILDSCRIPT"
>>>        ln -s "${BUILDFILE}" "${srclinks}/${pkgbase}/${BUILDSCRIPT}"
>>>  -       if [ -n "$install" ]; then
>>> -               if [ -f $install ]; then
>>> -                       msg2 "$(gettext "Adding install script...")"
>>> -                       ln -s "${startdir}/$install"
>>> "${srclinks}/${pkgbase}/"
>>> -               else
>>> -                       error "$(gettext "Install scriptlet (%s) does not
>>> exist.")" "$install"
>>> +       local install_files
>>> +       install_files=$(grep "install=" "$BUILDSCRIPT")
>>> +       for pkg in ${pkgname[@]}; do
>>> +               install_files+=' '
>>> +               install_files+=$(echo $install_files |sed
>>> "s/\$pkgname/$pkg/"|sed "s/\${pkgname}/$pkg/")
>>> +               install_files=$(eval echo $install_files |tr '[:blank:]'
>>> '\n'|sort |uniq)
>>> +       done
>> This is too inclusive.  If I have one package with an install file
>> $pkgname.install, then it will add an install file $pkg.install for every
>> $pkg in $pkgname.  Some of these may not exist which will lead to trouble
>> below when the files presence is tested for.
>
> I copied this from makechrootpkg.  I just tested it when a package in
> a split package doesn't use the install script and it works.

So, some background to my comment.

With split packages (currently) $pkgname within the package_foo()
functions is just the first element of the ${pkgname[@]} array.  This
may be quite different than expected and I think there is a bug report
to allow the use of $pkgname within those functions and have it actually
provide the name of the package being packaged.

Currently, that means you can skip the whole "for pkg in" loop and just
do a single sed as $pkgname will always be the first element in the array.

However, if having $pkgname equal to "foo" within the "package_foo()"
functions is implemented, you could have:

pkgname=('foo' 'bar')

package_foo() {
install=$pkgname.install
}

package_bar() {
#install=none
}

With your patch you would end up with:
install_files=('foo.install' 'bar.install')
which is wrong and would error out on missing bar.install.

So, there is currently too much looping going on here but in the future
this may break completely.

As I said, I am not sure what the fix is here.

Allan

[Eric Bélanger-2]

On Fri, Nov 6, 2009 at 11:43 AM, Allan McRae <allan@...> wrote:

> Eric Bélanger wrote:
>>
>> On Fri, Nov 6, 2009 at 10:36 AM, Allan McRae <allan@...> wrote:
>>>
>>> Eric Bélanger wrote:
>>>>
>>>> In some case, the install script of split packages was not included in
>>>> the
>>>> sourceballs. This was due because split packages can specify their
>>>> install
>>>> script in the package functions.  They also can use several install
>>>> scripts.
>>>>
>>> Please add newlines to the commit message to keep them ~80 characters
>>> wide.
>>
>> OK.  I'll try to remember next time.
>>
>>> Note that this will need done for changelogs too as they are now handled
>>> like install files.
>>
>> ChangeLog are not  included in sourceball   even for non split
>> package: $changelog doesn't get defined.  This will need to be fixed.
>> We can  test if there is a ChangeLog file in $startdir and if so we
>> include it. I could make a patch.
>>
>
> On the master branch, you are required to add "changelog=<file>" to include
> a changelog in your package.  That was added to allow split packages to have
> different changelogs in different packages.
>
>
>>>> Signed-off-by: Eric Bélanger <snowmaniscool@...>
>>>> ---
>>>>  scripts/makepkg.sh.in |   25 ++++++++++++++++++-------
>>>>  1 files changed, 18 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
>>>> index 8fc7c82..b77cc68 100644
>>>> --- a/scripts/makepkg.sh.in
>>>> +++ b/scripts/makepkg.sh.in
>>>> @@ -1060,14 +1060,25 @@ create_srcpackage() {
>>>>       msg2 "$(gettext "Adding %s...")" "$BUILDSCRIPT"
>>>>       ln -s "${BUILDFILE}" "${srclinks}/${pkgbase}/${BUILDSCRIPT}"
>>>>  -       if [ -n "$install" ]; then
>>>> -               if [ -f $install ]; then
>>>> -                       msg2 "$(gettext "Adding install script...")"
>>>> -                       ln -s "${startdir}/$install"
>>>> "${srclinks}/${pkgbase}/"
>>>> -               else
>>>> -                       error "$(gettext "Install scriptlet (%s) does
>>>> not
>>>> exist.")" "$install"
>>>> +       local install_files
>>>> +       install_files=$(grep "install=" "$BUILDSCRIPT")
>>>> +       for pkg in ${pkgname[@]}; do
>>>> +               install_files+=' '
>>>> +               install_files+=$(echo $install_files |sed
>>>> "s/\$pkgname/$pkg/"|sed "s/\${pkgname}/$pkg/")
>>>> +               install_files=$(eval echo $install_files |tr '[:blank:]'
>>>> '\n'|sort |uniq)
>>>> +       done
>>>
>>> This is too inclusive.  If I have one package with an install file
>>> $pkgname.install, then it will add an install file $pkg.install for every
>>> $pkg in $pkgname.  Some of these may not exist which will lead to trouble
>>> below when the files presence is tested for.
>>
>> I copied this from makechrootpkg.  I just tested it when a package in
>> a split package doesn't use the install script and it works.
>
> So, some background to my comment.
>
> With split packages (currently) $pkgname within the package_foo() functions
> is just the first element of the ${pkgname[@]} array.  This may be quite
> different than expected and I think there is a bug report to allow the use
> of $pkgname within those functions and have it actually provide the name of
> the package being packaged.
>
> Currently, that means you can skip the whole "for pkg in" loop and just do a
> single sed as $pkgname will always be the first element in the array.
>
> However, if having $pkgname equal to "foo" within the "package_foo()"
> functions is implemented, you could have:
>
> pkgname=('foo' 'bar')
>
> package_foo() {
> install=$pkgname.install
> }
>
> package_bar() {
> #install=none
> }
>
> With your patch you would end up with:
> install_files=('foo.install' 'bar.install')
> which is wrong and would error out on missing bar.install.
>
> So, there is currently too much looping going on here but in the future this
> may break completely.
>
> As I said, I am not sure what the fix is here.
>
> Allan
>

I see.

I think the install and changelog files should be treated differently
than the source so I'm not to warm about putting them in the source
array. How about having them in their own arrays:

install=(foo.install bar.install)
changelog=(ChangeLog.foo ChangeLog.bar)
source=(...)

We might want to change the array names a bit, e.g. installs=() and
changelogs=()

In the package functions,  you specify the one that you want:

package_foo() {
  install=$pkgname.install
  changelog=ChangeLog.$pkgname
}

 package_bar() {
   install=$pkgname.install
  changelog=ChangeLog.$pkgname
 }

assuming you do the $pkgname change you suggested above.  Then, when
you want a list of those files (to make sourceball, copy to chroot,
whatever) you just loop through the arrays like we already do for
pkgname. So no more need of these crazy grep and sed to get this
information.

[Xavier Chantry-2]

On Fri, Nov 6, 2009 at 6:08 PM, Eric Bélanger <snowmaniscool@...> wrote:
>
> assuming you do the $pkgname change you suggested above.  Then, when
> you want a list of those files (to make sourceball, copy to chroot,
> whatever) you just loop through the arrays like we already do for
> pkgname. So no more need of these crazy grep and sed to get this
> information.
>
>

We may want to do that for all pkgbuild variables.
Aaron and I discussed about using bash associative array.
But this still requires some more brainstorming and work :)

This is the second point that Allan made there :

> I do not know how to fix that issue... Two potential options are:
> 1) add install/changelog files to the source array
> 2) change the split PKGBUILD format to allow easier evaluation of variables within split packages.
> Both have their drawbacks.

[Dan McGee]

On Fri, Nov 6, 2009 at 11:38 AM, Xavier <shiningxc@...> wrote:

> On Fri, Nov 6, 2009 at 6:08 PM, Eric Bélanger <snowmaniscool@...> wrote:
>>
>> assuming you do the $pkgname change you suggested above.  Then, when
>> you want a list of those files (to make sourceball, copy to chroot,
>> whatever) you just loop through the arrays like we already do for
>> pkgname. So no more need of these crazy grep and sed to get this
>> information.
>>
> We may want to do that for all pkgbuild variables.
> Aaron and I discussed about using bash associative array.
> But this still requires some more brainstorming and work :)

This patch is on hold for the time being yet? Mostly just cleaning out
the inbox here but wasn't sure from the discussion whos turn it is to
act.

> This is the second point that Allan made there :
>
>> I do not know how to fix that issue... Two potential options are:
>> 1) add install/changelog files to the source array
>> 2) change the split PKGBUILD format to allow easier evaluation of variables within split packages.
>> Both have their drawbacks.

[Xavier Chantry-2]

On Mon, Nov 9, 2009 at 5:28 AM, Dan McGee <dpmcgee@...> wrote:

> On Fri, Nov 6, 2009 at 11:38 AM, Xavier <shiningxc@...> wrote:
>> On Fri, Nov 6, 2009 at 6:08 PM, Eric Bélanger <snowmaniscool@...> wrote:
>>>
>>> assuming you do the $pkgname change you suggested above.  Then, when
>>> you want a list of those files (to make sourceball, copy to chroot,
>>> whatever) you just loop through the arrays like we already do for
>>> pkgname. So no more need of these crazy grep and sed to get this
>>> information.
>>>
>> We may want to do that for all pkgbuild variables.
>> Aaron and I discussed about using bash associative array.
>> But this still requires some more brainstorming and work :)
>
> This patch is on hold for the time being yet? Mostly just cleaning out
> the inbox here but wasn't sure from the discussion whos turn it is to
> act.
>

I think the next act is to explore more the idea of an alternative
split format, to know if it is doable and solves all our problems.
The problem is that this should be done before more tool support for
split pkgbuilds is done.