[Aptitude-devel] Pending removal of libept, fixing of debtags issues (was: Preview of work refactored from experimental-0.6.9)

[Daniel Hartwig]

On 10 February 2014 09:20, Daniel Hartwig <mandyke at gmail.com> wrote:
> 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.

Updated with feedback (below) and completed, including remaining fixes
to tag handling from experimental-0.6.9.  This branch is ready to go,
missing only updates to NEWS for user-visible and other significant
changes.

This time next week I will merge it (with NEWS update) for the next 0.6 release.

>>
>>
>> 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.
>

Done using apt.conf variable Apt-Xapian-Index::Index at runtime.
Vendors can customize this system-wide by placing files in
/etc/apt/apt.conf.d.  As this is an external resource (unlike
package-state-loc and lock-loc), this is the more appropriate place to
configure it.

>> - 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.
>

Not yet.  Will wrap boost::shared_ptr around these and check
performance over the week.

>> - 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)


Regards