Re: Bug#748936: apt doesnt understand architecture wildcards
Hi Julian,
Thanks for taking a look at this.
> On 8 Jan 2017, at 21:42, Julian Andres Klode <jak@debian.org> wrote:
>> On Sun, Jan 08, 2017 at 01:33:08AM +0000, James Clarke wrote:
>>> On Mon, Feb 01, 2016 at 12:05:18AM +0100, David Kalnischkies wrote:
>>>> On Wed, Jan 20, 2016 at 01:39:45PM +0000, Colin Watson wrote:
>>>>> On Wed, Jan 20, 2016 at 12:31:52PM +0100, Balint Reczey wrote:
>>>>>> On 06/04/2014 03:41 AM, Guillem Jover wrote:
>>>>>> * Other programs could “easily” use dpkg-architecture to check for
>>>>>> identity or a match. (This poses problems for programs that do not
>>>>>
>>>>> I think making apt call dpkg-architecture for matching would be a good
>>>>> way of ensuring consistency with dpkg. Caching the results in a hash
>>>>> table would make matching even faster than it is currently.
>>>>
>>>> dpkg-architecture is in dpkg-dev, so not reliably usable at run-time.
>>>
>>> Also, apt is trying to remain largely independent of the low-level
>>> package manager, so bluntly depending on it wouldn't be good, but …
>>>
>>> … apt could survive this (for now) as these architecture specifications
>>> can at the moment only be encountered in
>>> a) build-dependencies of source packages
>>> (effecting via python-apt also presumably stuff like dak)
>>> b) the commandline like 'apt install libfoo:linux-* foo:linux-any'
>>> (and aptitude uses it, too, for this)
>>>
>>> So, we could do that without too much pain, while keeping a fallback
>>> around for cases in which we don't have dpkg-architecture around for
>>> whatever reason as it doesn't effect apts primary operation (but might
>>> effect the primary operation of other tools which would need to be
>>> tested first).
>>>
>>>
>>> I wonder through if we aren't creating the debian version of a tar bomb
>>> (xkcd#1168) and to illustrate that a little quiz:
>>>
>>> dpkg-architecture -ai386 -ii386 true
>>> dpkg-architecture -ai386 -ilinux-i386 true
>>> dpkg-architecture -ai386 -iany-i386 true
>>> dpkg-architecture -aarmhf -iarmhf true
>>> dpkg-architecture -aarmhf -ilinux-armhf true
>>> dpkg-architecture -aarmhf -iany-armhf false
>>> dpkg-architecture -aarmhf -iarm false
>>> dpkg-architecture -aarmhf -ilinux-arm false
>>> dpkg-architecture -aarmhf -iany-arm true
>>>
>>> Now, given we want to go to <libc>-<kernel>-<cpu> lets see:
>>> dpkg-architecture -aarmhf -iany-linux-arm true
>>> dpkg-architecture -aarmhf -iany-any-arm true
>>> dpkg-architecture -aarmhf -ignu-any-arm false
>>> dpkg-architecture -aarmhf -ignueabihf-any-arm true
>>>
>>> And to cool off, think about what matches any-arm, linux-any, and if
>>> gnu-any is even supported and if what that matches…
>>>
>>>
>>> Truth be told, I would have died on 'any-armhf' already even through
>>> that is "obvious" as 'linux-armhf' is interpreted as a literal
>>> architecture, while 'any-armhf' is a pattern (just that neither dpkg nor
>>> the policy highlight that such a difference exists explicitly).
>>>
>>> Anyway, I somehow doubt it will be a good idea to trouble mere mortals
>>> with the difference between gnu and gnueabihf for matching proposes, so
>>> I wonder why we have to trouble them with the difference of armhf and
>>> arm depending on if that specification is a literal architecture or an
>>> architecture pattern – especially if the two are different only for some
>>> architectures… I would personally be more happy with any-armhf working
>>> (and a special case letting arm match all of the arms).
>>>
>>>
>>> So, I actually like how apt behaves currently as it just makes more
>>> sense in my head to expand armhf to gnu-linux-armhf and match it against
>>> gnu-any-armhf instead of gnueabihf-linux-arm and and gnueabihf-any-arm,
>>> but so be it – it tends to be more important to have a consistent answer
>>> in Debian than to keep me sane… (yeah, I know, that triplet makes
>>> perfect sense if you know history, Debian and arm – I just don't).
>>>
>>>
>>> I am therefore going to happily accept any patch flying my way
>>> implementing architecture wildcards differently if need be, but I am not
>>> going to do it myself mainly because I expect that to have fallout – not
>>> in apt, but in things using apt – and I don't have the energy (or the
>>> rights) to deal with such things efficiently.
>>
>> Hi David,
>> I actually ran into this bug in the real world: elfutils has a
>> Build-Depends: gcc-multilib [any-amd64], which includes x32
>> (x32-gnu-linux-amd64), but apt build-dep didn't install it, so
>> dpkg-checkbuilddeps errored out when building. I agree some of this
>> seems crazy, but it's even more crazy to have apt and dpkg disagree IMO.
>>
>> I have attached an initial proof-of-concept[0] patch for apt to embed the
>> list of architecture tuples at build-time. It's not especially pretty,
>> but it passes the now-modified test suite run during the build
>> (integration tests not yet run), which includes testing x32 and arm
>> behave like they do in dpkg. I also haven't actually tested that
>> "apt build-dep elfutils" on x32 installs gcc-multilib, but the testsuite
>> success makes me pretty confident. Thoughts?
>
> This introduces a hard build dependency on dpkg. I guess that's sort
> of OK, though, for now. We really should move the whole architecture
> pattern matching into apt-pkg/deb at some point though I think (obviously
> we don't have another packaging system like rpm yet, but I don't want to give
> up hope...).
You can always make the table empty for non-dpkg, and it should function like
it used to, except with quadruplets rather than triplets. But yes, moving it to
be deb-specific makes sense.
> That said, we need to deal with our CI which still runs on trusty, as travis
> does not provide newer images. Not sure if we can upgrade dpkg there (we have
> the permissions, not sure if it works, though).
Hm, I can't see a way around that without storing a copy of dpkgs's table
inside the *source* package.
> As an additional extension to the spec, we might want to accept * everywhere
> inplace of any for compatibility purposes. I think this solves most issues.
Ok, that's easy.
>> Regards,
>> James
>>
>> [0] Hence no patch tag for the bug
>
> [...]
>> +extern std::unordered_map<std::string, std::vector<std::string>> const ArchToTupleMap;
>
> Maybe we should store std::array<std::string,4> instead of a vector? And maybe APT::StringView
> instead of strings? (at least the values, I don't think they work as keys...).
Sure; that has the benefit of being fixed-length, whereas currently things will
silently break if they're not quadruplets.
>> diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc
>> index cf8feb970..9216bab15 100644
>> --- a/apt-pkg/contrib/strutl.cc
>> +++ b/apt-pkg/contrib/strutl.cc
>> @@ -27,6 +27,7 @@
>> #include <locale>
>> #include <sstream>
>> #include <string>
>> +#include <sstream>
>> #include <vector>
>>
>> #include <stddef.h>
>> @@ -1319,6 +1320,17 @@ vector<string> StringSplit(std::string const &s, std::string const &sep,
>> }
>> return split;
>> }
>> +
>> +std::string StringJoin(std::vector<std::string> list, const std::string &sep)
>> +{
>> + std::ostringstream oss;
>> + for (auto it = list.begin(); it != list.end(); it++)
>> + {
>> + if (it != list.begin()) oss << sep;
>> + oss << *it;
>> + }
>> + return oss.str();
>> +}
>> /*}}}*/
>> // RegexChoice - Simple regex list/list matcher /*{{{*/
>> // ---------------------------------------------------------------------
>> diff --git a/apt-pkg/contrib/strutl.h b/apt-pkg/contrib/strutl.h
>> index 918ac89c7..b23ce914b 100644
>> --- a/apt-pkg/contrib/strutl.h
>> +++ b/apt-pkg/contrib/strutl.h
>> @@ -131,6 +131,8 @@ std::vector<std::string> StringSplit(std::string const &input,
>> std::string const &sep,
>> unsigned int maxsplit=std::numeric_limits<unsigned int>::max()) APT_CONST;
>>
>> +std::string StringJoin(std::vector<std::string> list, const std::string &sep);
>> +
>
> I guess it makes some sense to extract the new function into its own commit
> (makred with Gbp-Dch: ignore), but it's not really a big deal.
Easily done.
>> --- a/debian/control
>> +++ b/debian/control
>> @@ -67,6 +68,7 @@ Breaks: appstream (<< 0.9.0-3~), apt (<< 1.1~exp14), libapt-inst1.5 (<< 0.9.9~)
>> Recommends: apt (>= ${binary:Version})
>> Section: libs
>> Provides: libapt-pkg (= ${binary:Version})
>> +Built-Using: ${sourcedep:libdpkg-perl}
>
> We should probably avoid this. That's telling people that we need
> to binNMU apt with every dpkg upload, and dak to keep the version
> apt was built with around (as it's meant for GPL compliance, not
> generic built-using information).
Ah, indeed, that's too strong a requirement, given that the arch tables won't
be changing that frequently. I guess it can be dropped;you just have to be
more careful when new arches are added.
>> +++ b/generate-tuple-table
>> @@ -0,0 +1,29 @@
>> +#!/usr/bin/perl -w
>> +
>> +use strict;
>> +use warnings;
>> +
>> +use Dpkg::Arch qw(get_valid_arches debarch_to_debtuple);
>> +
>> +my $filename = shift or die "Usage: $0 <output>";
>> +
>> +my $output;
>> +open($output, '>', $filename) or die "Cannot open $filename: $!";
>> +
>> +print $output "#include <string>\n";
>> +print $output "#include <unordered_map>\n";
>> +print $output "#include <vector>\n";
>> +print $output "\n";
>> +print $output "#include \"macros.h\"";
>> +print $output "\n";
>> +print $output "namespace APT {\n";
>> +print $output " APT_HIDDEN extern std::unordered_map<std::string, std::vector<std::string>> const ArchToTupleMap = {\n";
>
> No extern keyword here, I'd say (it's implicit, and not really the normal style).
It's not implicit. The "const" makes it default to static linkage, unlike C. I
had a lot of fun finding that out for myself...
gcc doesn't seem to like compiling this; there are a lot of unsafe loop
optimisations, presumably because of whatever gcc turns the initialiser list
into. Perhaps those warnings (and any other relevant flags) should be disabled
for the generated file?
Regards,
James
Reply to: