Re: Bug#473461: apt-get should also have a --no-triggers option

[Daniel Burrows <dburrows@debian.org>]

  I'll try to comment, although I'm not very familiar with the dpkgpm
code.

> === modified file 'apt-pkg/deb/dpkgpm.cc'
> --- apt-pkg/deb/dpkgpm.cc	2008-01-31 09:06:00 +0000
> +++ apt-pkg/deb/dpkgpm.cc	2008-05-28 12:33:24 +0000
> @@ -526,6 +526,11 @@
>  
>     if (RunScriptsWithPkgs("DPkg::Pre-Install-Pkgs") == false)
>        return false;
> +
> +   // support subpressing of triggers processing for special
> +   // cases like d-i that runs the triggers handling manually

s/subpressing/suppressing/ :-)

  Maybe say something like this?

    // Insert dummy entries into the list of things to do that represent
    // the action of running the triggers.

> +   if(_config->FindB("DPkg::NoTriggers",false) == false)
> +      List.push_back(Item(Item::Triggers,PkgIterator()));
>     
>     // map the dpkg states to the operations that are performed
>     // (this is sorted in the same way as Item::Ops)
> @@ -572,6 +577,8 @@
>     // and the PackageOpsTranslations (human readable strings)
>     for (vector<Item>::iterator I = List.begin(); I != List.end();I++)
>     {
> +      if((*I).Pkg.end()) 
> +	     continue;

  I would add something like

     // Items representing a global action such as "run all pending
     // triggers" aren't associated with a particular package's state
     // and so they shouldn't go in the map of operations to be
     // performed on packages.

  I don't really see what we're doing with PackageOpsDone, so I'm not
sure this is right.

>        string name = (*I).Pkg.Name();
>        PackageOpsDone[name] = 0;
>        for(int i=0; (DpkgStatesOpMap[(*I).Op][i]).state != NULL;  i++) 
> @@ -649,6 +656,7 @@
>  	 
>  	 case Item::Configure:
>  	 Args[n++] = "--configure";
> +	 Args[n++] = "--no-triggers";

  Should that be protected by a configuration check against Dpkg::NoTriggers?

>  	 Size += strlen(Args[n-1]);
>  	 break;
>  	 
> @@ -658,6 +666,11 @@
>  	 Args[n++] = "--auto-deconfigure";
>  	 Size += strlen(Args[n-1]);
>  	 break;
> +
> +	 case Item::Triggers:
> +	 Args[n++] = "--configure";
> +	 Args[n++] = "--pending";
> +	 break;
>        }
>        
>        // Write in the file or package names
> @@ -675,6 +688,8 @@
>        {
>  	 for (;I != J && Size < MaxArgBytes; I++)
>  	 {
> +	    if(I->Pkg.end())
> +	       continue;

  Shouldn't we break out in this case and run triggers after we've
executed actions through I?  (note: I'm not sure I understand this
loop; it seems to me like we should have "&& I->Op == J->Op" in the
condition, to make sure we don't, e.g., conflate packages being
purged with packages being removed)

  If this is right, I think it deserves a comment explaining why.  (both
the existing code and the new code, actually)  Since I don't understand
why, I can't write it. :-)



  And on an unrelated note: is there a reason we rely on the g++ extension
of dynamically sized stack arrays in this method?

  Daniel