Re: [patch] EDSP 0.5 w/ multi-arch support for external solvers

[David Kalnischkies <kalnischkies+debian@gmail.com>]

Hi,

On Mon, Nov 4, 2013 at 2:11 PM, Stefano Zacchiroli <zack@debian.org> wrote:
> Can you please merge these patches and/or let me know which changes I
> should apply to make them suitable for merge? I can also open a
> wishlist bug report for this, if you want me to.

I had commented some parts on IRC before this series. Maybe they were
lost in transit, so let me repeat them … I am happy about the prospect of
EDSP becoming multi-arch aware, and I am mostly happy with the proposed
changes but let me voice a few concerns non-the-less:


On Mon, Nov 4, 2013 at 2:11 PM, Stefano Zacchiroli <zack@debian.org> wrote:
> diff --git a/apt-pkg/edsp.cc b/apt-pkg/edsp.cc
> index 38a29fc..93e8176 100644
> --- a/apt-pkg/edsp.cc
> +++ b/apt-pkg/edsp.cc
> @@ -234,6 +234,10 @@ bool EDSP::WriteRequest(pkgDepCache &Cache, FILE* output, bool const Upgrade,
>         fprintf(output, " %s", a->c_str());
>     fprintf(output, "\n");
>
> +   if (_config->Exists("Commandline::AsString") == true)
> +       fprintf(output, "Command-Line: %s\n",
> +              _config->Find("Commandline::AsString").c_str());
> +
>     if (del.empty() == false)
>        fprintf(output, "Remove: %s\n", del.c_str()+1);
>     if (inst.empty() == false)

I think I know why you want that, but I don't think its a good idea.

You want to know for which packages the user has chosen a specific
version vs. for which packages the solver is "free" to choose one, right?
(assuming there is actually a well defined difference)

Extracting this from the command line will become horrible very very soon.
Which ones do you understand: foo/sid foo=1.1-1 foo=1.* ^fo+.* foo:arm*
and do you know the difference between foo=newest and foo/newest ?
(and newest is of course not a normal release…)

Also, this setting is set by ALL libapt-pkg users, so mix in e.g.
aptitude, aptdaemon and your self-written python-apt script.
(Not sure ATM if aptitude will actually use EDSP if set,
 but at least many other frontends will)


> diff --git a/apt-pkg/edsp.cc b/apt-pkg/edsp.cc
> index 70b3654..1762fd7 100644
> --- a/apt-pkg/edsp.cc
> +++ b/apt-pkg/edsp.cc
> @@ -104,11 +104,18 @@ void EDSP::WriteScenarioVersion(pkgDepCache &Cache, FILE* output, pkgCache::PkgI
>     else if ((Ver->MultiArch & pkgCache::Version::Same) == pkgCache::Version::Same)
>        fprintf(output, "Multi-Arch: same\n");
>     signed short Pin = std::numeric_limits<signed short>::min();
> -   for (pkgCache::VerFileIterator File = Ver.FileList(); File.end() == false; ++File) {
> -      signed short const p = Cache.GetPolicy().GetPriority(File.File());
> +   std::set<string> Releases;
> +   for (pkgCache::VerFileIterator I = Ver.FileList(); I.end() == false; ++I) {
> +      pkgCache::PkgFileIterator File = I.File();
> +      signed short const p = Cache.GetPolicy().GetPriority(File);
>        if (Pin < p)
>          Pin = p;
> +      if ((File->Flags & pkgCache::Flag::NotSource) != pkgCache::Flag::NotSource)
> +        Releases.insert(File.RelStr());
>     }
> +   fprintf(output, "APT-Release:\n");
> +   for (std::set<string>::iterator R = Releases.begin(); R != Releases.end(); ++R)
> +      fprintf(output, " %s\n", R->c_str());
>     fprintf(output, "APT-Pin: %d\n", Pin);
>     if (Cache.GetCandidateVer(Pkg) == Ver)
>        fprintf(output, "APT-Candidate: yes\n");

Have you tested this with (flat) archives without a Release file?
I have the feeling that those return an empty string, which is probably
going to confuse EDSP readers. Maybe don't insert empty strings into
Releases set and don't print 'APT-Release:' if Releases is empty.


Best regards

David Kalnischkies