Re: [Tracker] What to do with libtracker-common?

[Martyn Russell <martyn lanedo com>]

On 07/10/14 13:14, Carlos Garnacho wrote:
> Hey,

Hi Carlos,

> On Mon, Sep 29, 2014 at 11:55 AM, Martyn Russell <martyn lanedo com
> <mailto:martyn lanedo com>> wrote:
>
> <snip>
>
>     Breakdown of libtracker-common
>     ------------------------------
>
>     -> tracker-config-file.h
>
>         Used for all GSettings/configs, so libtracker-data, libtracker-fts,
>         tracker-store, and the miners.
>
>
> The only purpose of this nowadays is to ensure the transition from
> keyfiles to gsettings, and will never be used after the keyfile is
> migrated and deleted.
>
> IMO we should remove this and fully trust on GSettings API. Right now it
> can only hurt to people updating all the way from 0.8, and if any
> embedded system wants to use Tracker and has a trouble with
> gsettings/dconf, they will surely have the same problem with other parts
> of the stack, and can just use a different GSettings backend.

OK, The TrackerConfigFile and TrackerKeyFile classes are now gone *AND* 
I've migrated the config to GKeyFileSettingsBackend for cases where we 
have TRACKER_USE_CONFIG_FILES defined in the environment.

I've also written a xslt template to create man pages from GSettings 
gschemas which seems to work quite nicely - so people can see what 
options are possible.

Relevant commit is here:

https://git.gnome.org/browse/tracker/commit/?h=libtracker-common-cleanup&id=37109980abea4303b97c9cd13ea0ec1e2e6f2e00

> <snip>
>
>     -> tracker-file-utils.h
>
>         Used to handle operations like getting file size, mtime, etc and
>         also for locking files, calculating disk space remaining, etc. One
>         important reason for APIs like tracker_file_open() is to make sure
>         extractors open with O_NOATIME and to allow posix_fadvise() use
>         consistently.
>
>
> File locking should go, it's currently unused, and only plays well if
> you respect the locks, so it really isn't as useful.

Done.

The rest of that file is quite hard to split. Most of the 
tracker_path_*() APIs can be just pushed to tracker-miner-fs where 
they're used, but the others like tracker_file_system_*() are used by 
libtracker-data and extractors, ...

We really need a central library for these sorts of things, and lower 
level than libtracker-data I would say. Unless it makes sense to import 
this API / module into libtracker-data?

>     -> tracker-ioprio.h
>
>         Set the I/O priority to be lower than normal to avoid disk
>         clobbering. Used by tracker-extract and the miners.
>
>         Could actually be in libtracker-miner.
>
>
> I would think this is too specific API, I wouldn't expect any external
> TrackerMiner to thrash the filesystem as massively as tracker-miner-fs does.

You make a fair point, but actually, it's used right now in:

  tracker-miner-apps
  tracker-miner-fs (*)
  tracker-miner-user-guides
  tracker-extract (*)

* = most heavy users.

They're all miners too.
It certainly doesn't belong in libtracker-common.

Carlos, I would like to put this in libtracker-miner under 
tracker-performance.[ch] where other things like this can exist. 
Depending on other APIs we migrate here, we could just put this in 
libtracker-miner-private.

> If tracker-extract and tracker-miner-fs are kept in the same repo, that
> could be private API between both though.
>
>
>     -> tracker-keyfile-object.h
>
>         Internal and used exclusively by tracker-config-file.h. So wherever
>         that ends up, so should this module.
>
>
> Exactly, I think it deserves the same end...

This has gone now.

>     -> tracker-locale-gconfdbus.h
>     -> tracker-locale.h
>     -> tracker-meego.h
>
>         Used to notify and keep track of local changes which is needed to
>         re-create database collations because sorting can vary by locale
>         (among other things).
>
>         The -gconfdbus file is an implementation, which we might even be
>         able to remove by now? *Is anyone using still using GConf?*
>
>         The -meego file is use to translate and get the locale on Meego.
>         *Is anyone still using Meego?* Would like to remove this.
>
>
> Please let's remove this outdated stuff, I consider on-the-fly locale
> change handling something unachievable today, no matter how much we tape
> over it
> <snip>

It's a lot of code and I actually think the locale check (for 
consistency) is a good one - any comment Aleksander?

I would happily drop the older gconf/meego stuff, but general locale 
management is worth keeping I think.

>     -> tracker-os-dependant.h
>
>         The tracker_spawn*() API here is only used by libtracker-data and 2
>         extractors, mplayer and ps (where we use gunzip, which is quite a
>         bad way to do this).
>
>
> I really don't know why the PS extractor isn't using poppler... I would
> prefer us to stick to GStreamer and remove the mplayer module, it's
> practically unused in any downstream, after that it can be moved to
> libtracker-data, or just make it use g_spawn_async_with_pipes itself.

I will clean all of this up.

>         The tracker_memory_setrlimits() is only used by tracker-extract and
>         could be moved there directly or put in libtracker-extract.
>
>
> Please let's just remove this, brings in enough grief.

Yea, there is a bug about this, so I will actually consider cgroups (as 
per the bug) as part of this change or removal.

Bug is here: https://bugzilla.gnome.org/show_bug.cgi?id=737663

> <snip>
>
>
>     Possible solutions
>     ------------------

...

> 5. Move libtracker-common to a separate git module, set it up on the
> relevant repos through git submodule, compile it statically in these
> repos, and add a configure script to that submodule that fails and
> points people to the right place(s).

I am not sure about git submodules for this. At this point, I am 
thinking about merging whatever is left into libtracker-data and of 
course libtracker-sparql needs that, so everyone gets it AFAICS.

--
Regards,
Martyn

Founder & Director @ Lanedo GmbH.
http://www.linkedin.com/in/martynrussell