[Aptitude-devel] Preview of work refactored from experimental-0.6.9

[Daniel Hartwig]

On 10 February 2014 02:16, Manuel A. Fernandez Montecelo
<manuel.montezelo at gmail.com> wrote:
> 2014-02-09 11:27 Daniel Hartwig:
>> * dth/remove-libept
>>
>> Libept is suggested to be removed by its developer.  Aptitude only
>> uses it to read debtags database and initiate apt-xapian-index.  Both
>> of these tasks are trivial to perform without libept, so this branch
>> is moving towards doing that.
>>
>> Refactored with care from experimental.
>
>
> Review comments/suggestions:
>
> - the string "/var/lib/apt-xapian-index/index" [1], perhaps should be
>   set with a constant?  Or a common place with constant-like, like
>   config.h?  Or as these in configure script:
>   "--with-package-state-loc" and "--with-lock-loc"?
>

This file location is fixed (at least as far as the axi package is
concerned).  It does make sense to have it configurable.  It shall be
so.

> - in [2], since it's not a reference, returning from function does not
>   need to be const, and probably shouldn't be
>

To maintain compatibility with the libept interface, it is not a
reference presently.  Once ept is removed it will become a reference
to avoid copy overhead.  Most (all?) consumers of this information do
not modify the set.

Likewise, the member type becomes 'const tag' to avoid changes there.

> - perhaps should be handy to check or assert ranges before this (also
>   in [2])?  "return tagDB[pkg->ID];"
>

tagDB is either NULL or holds exactly PackageCount objects.  NULL is
already guarded.  This is pattern is standard in apt for extending the
package cache with new data; it is well tested and reliable.

PackageCount only changes if the Cache is reloaded.  Reloading the
Cache causes tagDB to be destroyed via a hook.

> [1]
> http://anonscm.debian.org/gitweb/?p=aptitude/aptitude.git;a=commitdiff;h=029963a0f9fce7a541f7b2721b17f8eeb591fd04
>
> [2]
> http://anonscm.debian.org/gitweb/?p=aptitude/aptitude.git;a=commitdiff;h=e92c39b806520434bcdf509ffbb5072b2f1fb34a
> const std::set<tag> aptitude::apt::get_tags(const pkgCache::PkgIterator
> &pkg)
>
>
>
>> The HEAD is not yet
>> finalized, and then there are a few more commits to make on top of
>> that (multi-arch optimization and reading tags from apt db when
>> debtags is not installed).
>
>
> (Similar questions at bottom, perhaps you can hold your reply to this
> part and wait until the end).
>
> It's not very clear to me what's needed in order to integrate this and
> to plan releases.
>
> Can we (and would you like to) integrate when "head is finalized", or
> only after everything is complete?
>

The current HEAD on this branch needs more work, the two interfaces
are not yet comparable, for example with errors reported.  I will
update when it is ready to merge.

One thing you can immediately observe is that this branch does not yet
remove libept or allow it to be disabled at configure time (disabled
at configure time is the next commit which is ready to go but I want
to rework HEAD some more first).

> I'm OK with anything that you decide, but as a general idea, the
> sooner the better.  And also I prefer not to pack too many changes in
> new releases.
>

This whole branch is one change: remove libept.  I have separated it
in to the logical steps required for ease of implementation and
review.


>> * dth/cachefile
>>
>> This is the most significantly changed part of wip-cmdline.  In the
>> older branch there was much duplication of code from libapt for
>> constructing CacheSets, due to aptitudes independent cachefile class.
>> Here it is derived directly from apts class and hence, less code
>> duplication.
>
>
> Yay \o/
>
>
> Review comments/suggestions:
>
> - Why the "if (1) { }" in [3]?
>

This is related to the preceding TODO.  That branch is placed in
anticipation of an alternate using apt-xapian-index to speed up the
search.

> - The style in [4] is a bit inconsistent within the file:
>
>   pkgCache::PkgIterator const &Pkg,
>   std::string const &pattern)
>   const bool was_empty = pci->empty();
>
>   Is this because it was copied from Apt code/interfaces, or some
>   other reason?
>

Yes, because this code is mostly copied from apt.  To make it easier
to apply updates and spot any differences, I have kept the style
similar to the original code.  In the future, changes on the apt side
will make it unnecessary to duplicate code like this.

>> This final commit entangles some unrelated behavioral changes which I
>> am pulling out in to separate commits.
>
>
> Same question as for the other branch: Do you want to integrate the
> changes so far in the next releases, before changing the behaviour
> with error reporting and the rest?
>

No.  At a minimum, the changes to error reporting must come before
dth/cachefile, as the behaviour on that branch is derived.  In the
original wip-cmdline this was all muddled up.

Presently I am preparing things so that some of the changes can be
pushed out gradually, however, all of them are more drastic than any
changes currently on master.

Most likely 0.7 will have to contain everything but the CacheSet
changes to cmdline (not in the preview branch, but seen on
wip-cmdline).  That change can wait until the following release.
After that I only intend to fix handling of package holds and some
tweaks to multiarch handling.

>
>
>> * On to 0.7 development
>>
>> Merging any of this work is sufficient to justify a bump to 0.7.  In
>> particular, the tighter error handling and fixups to command line
>> handling constitute a noticeable change to the interface.
>>
>> We can expect final drafts of the complete work by this time next week.
>
>
> I think that you mean that the work not integrated yet (command line,
> errors...) justifies bumping to 0.7.  I agree with that.
>

Correct.

> Before 0.7 though, I would like to do 0.6.8.5 (and more 0.6.8.x if
> needed) with the content of master (and possibly a few more small
> fixes), and then (or before) the changes of russian documentation and
> multi-arch in the Debian side of the package.  I could have done it
> this weekend, but nobody replied and I was busy with BTS instead.
>
> From my point of view, several or all of the changes that you commited
> so far to both branches are OK and do not add any big change,
> specially not as seen from the users.  So do you want to include them
> already in this release?
>

The dth/* branches are only preview at this time, and not yet suitable
for master.  At least the final commit on both branches will be
amended as work elsewhere proceeds (there are some TODO items).

I will prepare next release in a couple of hours.


If there are any comments to be made on the changes to command line
argument and error handling, as discussed in the commit logs on
wip-cmdline, please make them soon.  The semantics do not change, only
some reworking for tidiness and adapting to current master.  There was
some discussion on the mailing list, which resulted in the semantics
on that branch.  At the time, I performed a thorough investigation
comparing apt, aptitude, and other utilities, and believe these
semantics are reasonable to proceed with on the 0.7 development
series.

<http://lists.alioth.debian.org/pipermail/aptitude-devel/2012-June/002673.html>


Regards