Page MenuHomeSoftware Heritage
Differential D490 Diff 1494 swh/indexer/origin_head.py

Changeset View

Standalone View

View Options

swh/indexer/origin_head.py

  • This file was added.
# Copyright (C) 2018 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution
# License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information
import re
import click
import logging
from swh.indexer.indexer import OriginIndexer
class OriginHeadIndexer(OriginIndexer):
"""Origin-level indexer.
This indexer is in charge of looking up the revision that acts as the
"head" of an origin.
In git, this is usually the commit pointed to by the 'master' branch."""
ADDITIONAL_CONFIG = {
'storage': ('dict', {
'cls': 'local',
zackUnsubmitted
Done
Inline Actions

As this is going to be persisted in the DB, it's time to define some decent naming for the indexer tools.
This is the "metadata indexer" so we should use it somewhere.
Also, we should go hierarchically: metadata → {origin,content,revision,etc.}
The fact we take the "head" revision is an implementation detail, it's just how the "origin metadata indexer" currently works.
"indexer" can remain implicit, as it's what all the tools do.

Bottom line: let's go for "metadata-origin".

zack: As this is going to be persisted in the DB, it's time to define some decent naming for the…
'args': {
'db': 'service=swh-replica',
'objstorage': OriginIndexer.DEFAULT_CONFIG['objstorage'][1],
}
ardumontUnsubmitted
Done
Inline Actions

The default configuration should be targeting a remote storage [1], defaulting to a local one as per [2] for example.

[1] D372#inline-1828

[2] https://docs.softwareheritage.org/devel/getting-started.html#step-4-ingest-repositories

ardumont: The default configuration should be targeting a remote storage [1], defaulting to a local one…
vlorentzAuthorUnsubmitted
Done
Inline Actions

Sorry, I didn't mean to commit that.

vlorentz: Sorry, I didn't mean to commit that.
}),
'tools': ('dict', {
'name': 'swh-head-revision',
'version': '0.0.1',
'configuration': {},
}),
}
def filter(self, ids):
yield from ids
def persist_index_computations(self, results, policy_update):
pass # TODO
# Dispatch
def index(self, origin):
moraneggUnsubmitted
Done
Inline Actions

if you are keeping prints for testing, don't mind this comment.
If not, don't forget erasing them (I forget them a lot).

moranegg: if you are keeping prints for testing, don't mind this comment. If not, don't forget erasing…
origin_id = origin['id']
latest_snapshot = self.storage.snapshot_get_latest(origin_id)
moraneggUnsubmitted
Not Done
Inline Actions

i would rename method or comment with an explicit comment saying
that each origin type requires a different HEAD retrieval method

but i'm being picky..

moranegg: i would rename method or comment with an explicit comment saying that each origin type requires…
method = getattr(self, '_try_get_%s_head' % origin['type'], None)
if method is None:
method = self._try_get_head_generic
rev_id = method(latest_snapshot)
if rev_id is None:
return None
result = {
'origin_id': origin_id,
'revision_id': rev_id,
}
zackUnsubmitted
Not Done
Inline Actions

as per T1257 this is no longer git-specific, so this should be _try_get_head

zack: as per T1257 this is no longer git-specific, so this should be `_try_get_head`
return result
moraneggUnsubmitted
Not Done
Inline Actions

Here the result is without the metadata and will be kept in the DB as a pointer to the last seen revision.
do you want to elaborate this by launching and copying the revision_metadata? or will it be another component?

It seems a good compartmentalization.
Maybe an orchestrator who uses the OriginIndexer's results to launch the RevisionIndexer with the list of revision ids?

moranegg: Here the result is without the metadata and will be kept in the DB as a pointer to the last…
vlorentzAuthorUnsubmitted
Done
Inline Actions

+1 for using another component

vlorentz: +1 for using another component
vlorentzAuthorUnsubmitted
Done
Inline Actions

Now that I think about it, it may affect the search between the time the OriginHead indexer runs and updates the reference, and the time the revision metadata is copied.

We should probably discuss this further, outside a Diff inline comments

vlorentz: Now that I think about it, it may affect the search between the time the OriginHead indexer…
moraneggUnsubmitted
Not Done
Inline Actions

I agree.

I'm voting to not copy the metadata and keep only the reference (which might change quickly)
The intrinsic metadata found in a revision is a more permanent object than the one we can associate with an origin.

moranegg: I agree. I'm voting to not copy the metadata and keep only the reference (which might change…
# VCSs
def _try_get_git_head(self, snapshot):
try:
if isinstance(snapshot, dict):
branches = snapshot['branches']
if branches[b'HEAD']['target_type'] == 'revision':
return branches[b'HEAD']['target']
zackUnsubmitted
Not Done
Inline Actions

… for the same reason this should now be removed

zack: … for the same reason this should now be removed
except KeyError:
return None
def _try_get_hg_head(self, snapshot):
zackUnsubmitted
Not Done
Inline Actions

I'm pretty sure this regex should not be here. It's a naming convention that, I guess, is also implemented by the tarball loader, right?
If so it should be exposed as a class attribute by it and used from here, to avoid duplicating it around.
(Unless you've very valid reasons to duplicate it here; if so they should be documented in a comment here.)

zack: I'm pretty sure this regex should not be here. It's a naming convention that, I guess, is also…
vlorentzAuthorUnsubmitted
Done
Inline Actions

As far as I understand, the tarball loader does not try to parse the file name; it only downloads a tarball from an URL, extracts it, and forwards it to the dir loader (which doesn't seem to parse names either).

vlorentz: As far as I understand, the tarball loader does not try to parse the file name; it only…
zackUnsubmitted
Not Done
Inline Actions

Fair enough. Thanks for checking.

zack: Fair enough. Thanks for checking.
pass # TODO, see https://forge.softwareheritage.org/T1189
# Tarballs
ardumontUnsubmitted
Not Done
Inline Actions

For the snapshot, possible target_type per loader:

  • mercurial: release, revision [1]
  • pypi: release, revision, alias (pointer to another release/revision mentioned in the snapshot) [2]
  • svn: revision [3]

[1] https://forge.softwareheritage.org/source/swh-loader-mercurial/browse/master/swh/loader/mercurial/loader.py$475-482

[2] https://forge.softwareheritage.org/source/swh-loader-pypi/browse/master/swh/loader/pypi/loader.py$239-271

[3] https://forge.softwareheritage.org/source/swh-loader-svn/browse/master/swh/loader/svn/loader.py$34-53

ardumont: For the snapshot, possible target_type per loader: - mercurial: release, revision [1] - pypi…
vlorentzAuthorUnsubmitted
Done
Inline Actions

What would the name of the "head" "branch" in mercurial be? "tip"?

vlorentz: What would the name of the "head" "branch" in mercurial be? "tip"?
ardumontUnsubmitted
Not Done
Inline Actions

default is the head branch.
tip is what they use to define it, i think.

ardumont: `default` is the head branch. `tip` is what they use to define it, i think.
_archive_filename_re = re.compile(
b'^'
b'(?P<pkgname>.*)[-_]'
b'(?P<version>[0-9]+(\.[0-9])*)'
b'(?P<preversion>[-+][a-zA-Z0-9.~]+?)?'
zackUnsubmitted
Not Done
Inline Actions

same for this method. (And hence maybe we do not need to expose the regex, but rather a filename parsing method from the tarball loader?)

zack: same for this method. (And hence maybe we do not need to expose the regex, but rather a…
b'(?P<extension>(\.[a-zA-Z0-9]+)+)'
b'$')
moraneggUnsubmitted
Not Done
Inline Actions

I'm not a real python programmer, so this is not my place to say
but the else after the except looks kinda misplaced (esthetically of course).

I think it's only me, so you can completely ignore this comment.

moranegg: I'm not a real python programmer, so this is not my place to say but the else after the except…
vlorentzAuthorUnsubmitted
Done
Inline Actions

It's a block that is run if the try: block did not raise an exception. I can drop the else: if you find it less readable.

vlorentz: It's a block that is run if the `try:` block did not raise an exception. I can drop the `else:`…
@classmethod
def _parse_version(cls, filename):
"""Extracts the release version from an archive filename,
to get an ordering whose maximum is likely to be the last
version of the software
>>> OriginHeadIndexer._parse_version(b'foo')
(-inf,)
>>> OriginHeadIndexer._parse_version(b'foo.tar.gz')
moraneggUnsubmitted
Not Done
Inline Actions

is 'alias' and 'revision' the only target_type possible?

moranegg: is 'alias' and 'revision' the only target_type possible?
vlorentzAuthorUnsubmitted
Done
Inline Actions

afaik, yes.

vlorentz: afaik, yes.
vlorentzAuthorUnsubmitted
Done
Inline Actions

psql does not agree:

softwareheritage=> select distinct target_type from snapshot_branch;
 target_type 
-------------
 
 content
 release
 revision
 alias
 directory

I'm not sure what to do with content and directory though. I'll create an issue later.

vlorentz: psql does not agree: ``` softwareheritage=> select distinct target_type from snapshot_branch…
(-inf,)
>>> OriginHeadIndexer._parse_version(b'gnu-hello-0.0.1.tar.gz')
(0, 0, 1, 0)
>>> OriginHeadIndexer._parse_version(b'gnu-hello-0.0.1-beta2.tar.gz')
(0, 0, 1, -1, 'beta2')
>>> OriginHeadIndexer._parse_version(b'gnu-hello-0.0.1+foobar.tar.gz')
(0, 0, 1, 1, 'foobar')
"""
res = cls._archive_filename_re.match(filename)
if res is None:
return (float('-infinity'),)
version = [int(n) for n in res.group('version').decode().split('.')]
if res.group('preversion') is None:
version.append(0)
else:
preversion = res.group('preversion').decode()
if preversion.startswith('-'):
version.append(-1)
version.append(preversion[1:])
elif preversion.startswith('+'):
zackUnsubmitted
Done
Inline Actions

We shouldn't have a default here, as there is no sane default.

If you want the list somewhere for ease of testing please put them in a REAME or use a local script.

zack: We shouldn't have a default here, as there is no sane default. If you want the list somewhere…
moraneggUnsubmitted
Done
Inline Actions

I used a default list with the RevisionMetatadaIndexer to test the component outside the MockStorage
and didn't think to delete it.
A script is a good alternative and adding a test to this or the next diff would be nice.

moranegg: I used a default list with the RevisionMetatadaIndexer to test the component outside the…
vlorentzAuthorUnsubmitted
Done
Inline Actions

I'll turn these into tests.

vlorentz: I'll turn these into tests.
version.append(1)
version.append(preversion[1:])
else:
assert False, res.group('preversion')
return tuple(version)
def _try_get_ftp_head(self, snapshot):
archive_names = list(snapshot['branches'])
max_archive_name = max(archive_names, key=self._parse_version)
r = self._try_resolve_target(snapshot['branches'], max_archive_name)
return r
# Generic
def _try_get_head_generic(self, snapshot):
# Works on 'deposit', 'svn', and 'pypi'.
try:
if isinstance(snapshot, dict):
branches = snapshot['branches']
except KeyError:
return None
else:
return (
self._try_resolve_target(branches, b'HEAD') or
self._try_resolve_target(branches, b'master')
)
def _try_resolve_target(self, branches, target_name):
try:
target = branches[target_name]
while target['target_type'] == 'alias':
target = branches[target['target']]
if target['target_type'] == 'revision':
return target['target']
except KeyError:
return None
@click.command()
@click.option('--origins', '-i', default=[
'git+https://github.com/SoftwareHeritage/swh-storage',
'git+https://github.com/SoftwareHeritage/swh-indexer',
'deposit+https://forge.softwareheritage.org/source/jesuisgpl/',
'ftp+rsync://ftp.gnu.org/gnu/3dldf',
'hg+https://gnuplotutils.googlecode.com/hg/',
'pypi+https://pypi.org/project/0-._.-._.-._.-._.-._.-._.-0/',
'pypi+https://pypi.org/project/limnoria/',
'svn+http://0-512-md.googlecode.com/svn/',
],
help='Origins to lookup, in the "type+url" format',
moraneggUnsubmitted
Not Done
Inline Actions

I don't mind keeping the main method with the defaults here for a few iterations before deleting it.

moranegg: I don't mind keeping the main method with the defaults here for a few iterations before…
multiple=True)
def main(origins):
rev_metadata_indexer = OriginHeadIndexer()
rev_metadata_indexer.run(origins, 'update-dups', parse_ids=True)
if __name__ == '__main__':
logging.basicConfig(level=logging.INFO)
main()
About · Developer information · Legal