Re: new apt for debian/experimental

[Michael Vogt <michael.vogt@ubuntu.com>]

Hello,

Matt was so kind to review some of the apt branches in
development. 

I asked him to review (from michael.vogt@ubuntu.com--2005) for
inclusion into apt--main:
apt--trust-cdrom--0
apt--progress-reporting--0
apt--ddtp--0
apt--auto-mark--0
apt--local-install--0

Here is our discussion, comments (and help) welcome :) 

On Tue, Jul 26, 2005 at 04:34:51PM -0700, Matt Zimmerman wrote:
> On Wed, Jul 06, 2005 at 07:28:02PM +0200, Michael Vogt wrote:
> > 
> > apt--trust-cdrom--0:
> > seems to make the life of the installer people a lot easier
> 
> Could you add documentation to this branch?  The changes are simple enough;
> I'm happy to merge it once it's documented.

Thanks, it's documented now in configure-index. It is set to "false"
by default.
 
> > apt--progress-reporting: 
> > because it is needed to hide the output of dpkg during the
> > base-install in stage2 of the installer
> 
> - What is the purpose of the "N_" macro?  Shouldn't these be simple
>   translatable strings?

N_(x) is a macro that is used to only mark strings as
translatable. _() is defined as #define _(x) gettext(x) (so it marks
and translates). N_() is usefull for eg static array
initializations. But it seems to be no longer needed with c++ so I
removed it.

> - Please use sizeof() for status_fd_buf, s, and anywhere else you are using
>   snprintf on a statically sized buffer

Thanks for pointing that out, fixed now. 

> - What happens if this read() fails?
> +        int len = read(_dpkgin, buf, 500);

Thanks, this is fixed now too. On error/EOF it will now waitpid() for
the child.

> - I suggest using an iostream for the pipe, to avoid issues with lines
>   potentially being broken across multiple read() calls

Standard c++ does have support for iostream on file descriptors (boost
has). I changed the code to use fdopen()/fgets() now to avoid any
broken line issues.

> > apt--ddtp:
> > to support translated descriptions
> 
> - I think you wrote "Tree" where you meant "Three".

Thanks, fixed.

> - The comment for pkgAcqIndexTrans could be more descriptive

Fixed, thanks.

> - What is void _dummy() ?

I have no idea. Every cache iterator class has it. It seems to be save
to remove, I guess it was used to work around problem in old g++
versions? Maybe someone from the deity list knowns?

> - The brace style in debListParser::Description_md5 should be made
>   consistent

Fixed. 

> - I'm not sure that the Description_md5 stuff is necessary at all; it could
>   very well be simpler and more efficient to do a string comparison.  What
>   do you think?

The md5sum is used to keep translation and description in sync. Apt
always has the english translation and each translation carries the
md5sum of the description it was translated against. Apt will now only
consider translated descriptions valid that match the md5sum of the
current packages description. This avoids that outdated/invalid
translated descriptions will be presented to the user. 

> - Is there any point in trying to retrieve a translation index for a
>   debSourcesIndex?

No, my bad. Removed. 

> - UseTranslation should be renamed; it sounds like an action rather than an
>   inquiry

Changed to TranslationsAvailable(), thanks.

> - What does the comment "approach: just get the first letter before the
>   underscore?!?" mean?  It doesn't seem to correspond to the code

Thanks. 

> - Brace style in pkgIndexFile::LanguageCode

Fixed.

> - I am not sure whether all of the cache handling is correct; I hate that
>   part of the code

I spend quite a bit of time testing/fixing this bits for the
code. It's possible that there are still bugs in it, but I think it
works pretty well now. And I can only agree that this bits of the code
are really messy :)

> - The code in apt-get.cc:DisplayRecord seems to assume that Description is
>   the last field in the buffer; I don't think this is always true (Bugs,
>   Origin, Task)

Yes, that was a oversight. Fixed now too. 

I also added support for apt-cdrom to retrieve the translations from
the cdrom.

> > apt--auto-mark: automatic dependency handling, only if it turns out to be
> > reasonable well working (I build test-packages on
> > p.u.c/~mvo/apt/auto-remove). It would make a lot of stuff easier (like
> > removing a language support package would remove the automatically
> > installed dependencies of it as well) and it's something usefull in
> > general. The principal idea is around in aptitude for some time and the
> > current implementation was developed in close cooperation with daniel
> > burrows. (this is part of the DependecnyManagment spec).
> 
> I can't think straight enough to review this right now.  Does it work?

It works for me and (at least) for dholbach. I asked for testing on
ubuntu-devel before I left for vacation but I got no feedback :/

The implementation is based on a slightly modified mark-n-sweep
algorithms from aptitude. It seperates the mark and the sweep phases a
bit more cleanly than aptitude. I exchanged some mails with Daniel
Burrows (thanks Daniel for your help!) about the design and he seems
to be ok with the new code in apt (there are two smaller issues in the
sweep code that are interessting for aptittude and that are not done
yet and there is a callback function that needs to become a propper
class).

I make use of the depcache internal tracking what packages are
installed automatically and which are not. This is different than
aptitude (but it seems to generally work). The nice thing is that
automatically installed packages will be tracked no matter what
frontend is used. It even works with python-apt.
 
> > A target of opportunity would be the apt--local-install branch because
> > it's a feature that people are asking for some time. But I don't feel very
> > comfortable with the implementation yet. Maybe libdpkg will make things
> > easier, but OTOH waiting for libdpkg may be waiting a bit too long ;)
> 
> I think I would prefer to merge the two libraries, rather than link one
> against the other.  What do you think?

Yes, that sounds good to me. This is something I would want to do
later because of the potential breakage that is involved when
combining the libraries. OTOH if you feel we should go forward here, I
can start working on this immediately.

Thanks,
 Michael

-- 
Linux is not The Answer. Yes is the answer. Linux is The Question. - Neo