[pacman-dev] [PATCH 4/5] makepkg: add soprovides support

[Florian Pritz]

On 19.01.2011 19:54, Dan McGee wrote:
> On Wed, Jan 19, 2011 at 11:13 AM, Florian Pritz <bluewind at xssn.at> wrote:
> 
> <Descriptive commit message usually goes here>
> 
> What does this do? Why? What is the generated format? This stuff needs
> to be here in permanent history so someone patching bugs 2 years from
> now can figure out the what and why.
> 
>> Support-by: brain0 <thomas at archlinux.org>
>> Support-by: GNU\caustic <Christoph.Schied at uni-ulm.de>
> Since the real names of these guys are so obvious, I'd prefer those
> are there rather than nicks.

Fixed locally.

>> +find_soprovides() {
>> +       local soprovides
>> +       find $pkgdir -type f -name \*.so\* | while read filename
> $pkgdir needs quotes, just like everywhere else it is used.
> 
>> +       do
>> +    if readelf -h "$filename" 2>/dev/null | grep -q '.*Type:.*DYN (Shared object file).*'; then
>> +                       soarch="$(objdump -a "$filename" 2>/dev/null | \
>> +                               sed -rn 's/.* file format elf[0-9]+-(.*)$/\1/p' | tr - _)_$(uname -s)"
>> +      sofile=$(readelf -d "$filename" 2>/dev/null | sed -nr 's/.*Library soname: \[(.*)\].*/\1/p')
>> +                       [ -z "$sofile" ] && sofile="$(basename "$filename")"
>> +
>> +                       soname=$(sed -rn 's/(.*)\.so.*/\1.so/p' <<< "$sofile")
>> +                       soversion=$(sed -rn 's/.*\.so\.(.*)/\1/p' <<< "$sofile")
> Please give examples in a resubmit of what is coming out of here- this
> is not super easy to follow.

Do you want comments in the code, one big comment above the function or
just an explanation in the commit message (will do that one anyway)?

> I'm also a bit concerned about:
> * the uses of sed (is -r portable? We use -n already, but not -r)
> * the inconsistencies between `sed -rn` and `sed -nr`

I'll just remove the -r then

> * running this in any non-C, non-en locale

Fixed locally.

> * introducing dependencies on readelf and objdump. At least elsewhere
> in the strip code, we use `file` to locate shared libraries. And you
> can get the SONAME bit out of objdump -p, which would at least limit
> this to one tool.

readelf and objdump are both in binutils, but I've noticed that objdump
doesn't support the file format generated by tcc although readelf does
and it's a valid ELF file afaict.

eu-objdump (elfutils) can't output that much, but it understands tcc's
format and outputs pretty nice architecture information. Sadly I have no
idea if that's portable.

Now I'm using readelf (binutils) and eu-objdump (elfutils). I think it's
fine since both are in core and you only need them for makepkg anyway.

> 
>> +                       if in_array "${soname}" ${provides[@]}; then
>> +                               if ! in_array "${soname}=${soversion}-${soarch}" ${soprovides[@]}; then
>> +                                       echo "${soname}=${soversion}-${soarch}"
>> +                                       soprovides=(${soprovides[@]} "${soname}=${soversion}-${soarch}")
> What's the reasoning behind the ${soarch} append? I suppose it might
> help with multilib, but something about this just doesn't seem right.

Yeah without that sodeps become useless for packages like wine (32+64bit
in multilib)

> It is most definitely not a valid pkgver (dash) or pkgrel (not a
> number).

The dash here just seperates pkgver from pkgrel.

Did a quick test with libc.so=6-x86_64_Linux as dependency and a package
called libc.so with that pkgver and pkgrel and it worked just fine.

> 
>> +                               fi
>> +                       fi
>> +    fi
>> +       done
>> +}
>> +
>>  write_pkginfo() {
>>        local builddate=$(date -u "+%s")
>>        if [[ -n $PACKAGER ]]; then
>> @@ -950,10 +972,24 @@ write_pkginfo() {
>>        [[ $depends ]]    && printf "depend = %s\n"    "${depends[@]}"
>>        [[ $optdepends ]] && printf "optdepend = %s\n" "${optdepends[@]}"
>>        [[ $conflicts ]]  && printf "conflict = %s\n"  "${conflicts[@]}"
>> -       [[ $provides ]]   && printf "provides = %s\n"  "${provides[@]}"
>>        [[ $backup ]]     && printf "backup = %s\n"    "${backup[@]}"
>>
>>        local it
>> +
>> +       soprovides=$(find_soprovides)
>> +       provides=("${provides[@]}" ${soprovides})
>> +
>> +       for it in "${provides[@]}"; do
>> +               if grep -q ".*\.so$" <<< "$it"; then
>> +                       if ! grep -q "\(^\|\s\)${it}=.*" <<< $soprovides; then
>> +                               error "$(gettext "Can't find library listed in \$provides: %s")" "$it"
> Do we really need to add 18 more forks to makepkg and use grep here?
> Bash has regexes, so it would prevent the cryptic use of <<<.

Fixed locally.

> I don't even know why we are doing this check, can you elaborate?

First I filter the entries from the PKGBUILD where you use something
like this:
provides=(libc.so)

Then I check if any of those removed entries is missing in the sodeps
array. In that case you have a sodep in your PKGBUILD that is not needed
and you should remove it.

>> +                               return 1
>> +                       fi
>> +               else
>> +                       echo "provides = $it"
>> +               fi
>> +       done
>> +
>>        for it in "${packaging_options[@]}"; do
>>                local ret="$(check_option $it)"
>>                if [[ $ret != "?" ]]; then
>> --
>> 1.7.3.5
> 


-- 
Florian Pritz -- {flo,bluewind}@server-speed.net

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://mailman.archlinux.org/pipermail/pacman-dev/attachments/20110121/059a3a14/attachment.asc>