Changeset View
Standalone View
swh/lister/maven/lister.py
- This file was added.
# Copyright (C) 2021 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 | |||||||||||||||||||||||||||||||||||||||||||||||||||||
from dataclasses import asdict, dataclass | |||||||||||||||||||||||||||||||||||||||||||||||||||||
import logging | |||||||||||||||||||||||||||||||||||||||||||||||||||||
import re | |||||||||||||||||||||||||||||||||||||||||||||||||||||
from typing import Any, Dict, Iterator, Optional | |||||||||||||||||||||||||||||||||||||||||||||||||||||
from urllib.parse import urljoin | |||||||||||||||||||||||||||||||||||||||||||||||||||||
import requests | |||||||||||||||||||||||||||||||||||||||||||||||||||||
from tenacity.before_sleep import before_sleep_log | |||||||||||||||||||||||||||||||||||||||||||||||||||||
from urllib3.util import parse_url | |||||||||||||||||||||||||||||||||||||||||||||||||||||
import xmltodict | |||||||||||||||||||||||||||||||||||||||||||||||||||||
from swh.lister.utils import throttling_retry | |||||||||||||||||||||||||||||||||||||||||||||||||||||
from swh.scheduler.interface import SchedulerInterface | |||||||||||||||||||||||||||||||||||||||||||||||||||||
from swh.scheduler.model import ListedOrigin | |||||||||||||||||||||||||||||||||||||||||||||||||||||
from .. import USER_AGENT | |||||||||||||||||||||||||||||||||||||||||||||||||||||
from ..pattern import CredentialsType, Lister | |||||||||||||||||||||||||||||||||||||||||||||||||||||
logger = logging.getLogger(__name__) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
RepoPage = Dict[str, Any] | |||||||||||||||||||||||||||||||||||||||||||||||||||||
vlorentz: Can you define this type better? A dataclass would be perfect. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsI didn't realise how much work it would involve at first, ahah. Sure it's better for documentation. Done. borisbaldassari: I didn't realise how much work it would involve at first, ahah. Sure it's better for… | |||||||||||||||||||||||||||||||||||||||||||||||||||||
@dataclass | |||||||||||||||||||||||||||||||||||||||||||||||||||||
class MavenListerState: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"""State of the MavenLister""" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
last_seen_doc: int = -1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"""Last doc ID ingested during an incremental pass | |||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
last_seen_pom: int = -1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"""Last doc ID related to a pom and ingested during | |||||||||||||||||||||||||||||||||||||||||||||||||||||
an incremental pass | |||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
class MavenLister(Lister[MavenListerState, RepoPage]): | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"""List origins from a Maven repository. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Maven Central provides artifacts for Java builds. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
It includes POM files and source archives, which we download to get | |||||||||||||||||||||||||||||||||||||||||||||||||||||
the source code of artifacts and links to their scm repository. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
This lister yields origins of types: git/svn/hg or whatever the Artifacts | |||||||||||||||||||||||||||||||||||||||||||||||||||||
use as repository type, plus maven types for the maven loader (tgz, jar).""" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
LISTER_NAME = "maven" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
def __init__( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
self, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
scheduler: SchedulerInterface, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
url: str, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
index_url: str = None, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
instance: Optional[str] = None, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
credentials: CredentialsType = None, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
incremental: bool = True, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actionsyes, keep it. You can drop the question in comment. ardumont: yes, keep it.
It's a required implementation detail for the instantiation of the lister. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
): | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"""Lister class for Maven repositories. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Args: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
url: main URL of the Maven repository, i.e. url of the base index | |||||||||||||||||||||||||||||||||||||||||||||||||||||
used to fetch maven artifacts. For Maven central use | |||||||||||||||||||||||||||||||||||||||||||||||||||||
https://repo1.maven.org/maven2/ | |||||||||||||||||||||||||||||||||||||||||||||||||||||
index_url: the URL to download the exported text indexes from. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Would typically be a local host running the export docker image. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
See README.md in this directory for more information. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
instance: Name of maven instance. Defaults to url's network location | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if unset. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
incremental: bool, defaults to True. Defines if incremental listing | |||||||||||||||||||||||||||||||||||||||||||||||||||||
is activated or not. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
self.BASE_URL = url | |||||||||||||||||||||||||||||||||||||||||||||||||||||
self.INDEX_URL = index_url | |||||||||||||||||||||||||||||||||||||||||||||||||||||
self.incremental = incremental | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actionsthey should not be in caps as they are not constants. vlorentz: they should not be in caps as they are not constants. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if instance is None: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
instance = parse_url(url).host | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsIt's done with the super() call just below (within the mother class, granted with the netloc but that should be close enough). ardumont: It's done with the super() call just below (within the mother class, granted with the netloc… | |||||||||||||||||||||||||||||||||||||||||||||||||||||
super().__init__( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
scheduler=scheduler, credentials=credentials, url=url, instance=instance, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
self.session = requests.Session() | |||||||||||||||||||||||||||||||||||||||||||||||||||||
self.session.headers.update( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
{"Accept": "application/json", "User-Agent": USER_AGENT,} | |||||||||||||||||||||||||||||||||||||||||||||||||||||
) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
def state_from_dict(self, d: Dict[str, Any]) -> MavenListerState: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
return MavenListerState(**d) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
def state_to_dict(self, state: MavenListerState) -> Dict[str, Any]: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
return asdict(state) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
@throttling_retry(before_sleep=before_sleep_log(logger, logging.WARNING)) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
def page_request(self, url: str, params: Dict[str, Any]) -> requests.Response: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
logger.info("Fetching URL %s with params %s", url, params) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
response = self.session.get(url, params=params) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if response.status_code != 200: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
logger.warning( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"Unexpected HTTP status code %s on %s: %s", | |||||||||||||||||||||||||||||||||||||||||||||||||||||
response.status_code, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
response.url, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
response.content, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
response.raise_for_status() | |||||||||||||||||||||||||||||||||||||||||||||||||||||
return response | |||||||||||||||||||||||||||||||||||||||||||||||||||||
def get_pages(self) -> Iterator[RepoPage]: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
""" Retrieve and parse exported maven indexes to | |||||||||||||||||||||||||||||||||||||||||||||||||||||
identify all pom files and src archives. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# Example of returned RepoPage's: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# [ | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# "type": "maven", | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# "url": "https://maven.xwiki.org/..-5.4.2-sources.jar", | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# "time": 1626109619335, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# "gid": "org.xwiki.platform", | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# "aid": "xwiki-platform-wikistream-events-xwiki", | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# "version": "5.4.2" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# }, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# "type": "scm", | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# "url": "scm:git:git://github.com/openengsb/openengsb-framework.git", | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# "project": "openengsb-framework", | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# }, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# ... | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# Download the main text index file. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
logger.info(f"Downloading text index from {self.INDEX_URL}.") | |||||||||||||||||||||||||||||||||||||||||||||||||||||
assert self.INDEX_URL is not None | |||||||||||||||||||||||||||||||||||||||||||||||||||||
response = requests.get(self.INDEX_URL, stream=True) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
response.raise_for_status() | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsCould you add a comment to give a high-level description of the format returned by this endpoint? (or link to such a description) vlorentz: Could you add a comment to give a high-level description of the format returned by this… | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsThe format is described in the README. Adding a link to the documentation inline. borisbaldassari: The format is described in the README. Adding a link to the documentation inline. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsThe link is missing, can you please add it. ardumont: The link is missing, can you please add it. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# Prepare regexes to parse index exports. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# Parse doc id. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# Example line: "doc 13" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
re_doc = re.compile(r"^doc (?P<doc>\d+)$") | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# Parse gid, aid, version, classifier, extension. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# Example line: " value al.aldi|sprova4j|0.1.0|sources|jar" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
re_val = re.compile( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
r"^\s{4}value (?P<gid>[^|]+)\|(?P<aid>[^|]+)\|(?P<version>[^|]+)\|" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
+ r"(?P<classifier>[^|]+)\|(?P<ext>[^|]+)$" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# Parse last modification time. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# Example line: " value jar|1626109619335|14316|2|2|0|jar" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
re_time = re.compile( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
r"^\s{4}value ([^|]+)\|(?P<mtime>[^|]+)\|([^|]+)\|([^|]+)\|([^|]+)" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
+ r"\|([^|]+)\|([^|]+)$" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actions
integrate this into the docstring ^ (dropping the comment) so ardumont: integrate this into the docstring ^ (dropping the comment) so
that makes it into the… | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsOf course, it's far better. Thanks. Also added the link to the README.md file in the current directory. borisbaldassari: Of course, it's far better. Thanks.
Also added the link to the README.md file in the current… | |||||||||||||||||||||||||||||||||||||||||||||||||||||
) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# Read file line by line and process it | |||||||||||||||||||||||||||||||||||||||||||||||||||||
out_pom: Dict = {} | |||||||||||||||||||||||||||||||||||||||||||||||||||||
jar_src: Dict = {} | |||||||||||||||||||||||||||||||||||||||||||||||||||||
doc_id: int = 0 | |||||||||||||||||||||||||||||||||||||||||||||||||||||
jar_src["doc"] = None | |||||||||||||||||||||||||||||||||||||||||||||||||||||
url_src = None | |||||||||||||||||||||||||||||||||||||||||||||||||||||
iterator = response.iter_lines(chunk_size=1024) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
for line_bytes in iterator: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# Read the index text export and get URLs and SCMs. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
line = line_bytes.decode(errors="ignore") | |||||||||||||||||||||||||||||||||||||||||||||||||||||
m_doc = re_doc.match(line) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsWhat kind of errors do you get here? vlorentz: What kind of errors do you get here? | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsDecode errors, see https://forge.softwareheritage.org/D6133#inline-45664 Should I add this link to the py file as well for reference? borisbaldassari: Decode errors, see https://forge.softwareheritage.org/D6133#inline-45664
Should I add this… | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actions
I agree with you on this. @douardda do you concur? vlorentz: > I have the feeling we should rather fail (throw an exception about decoding and end… | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsPlease, add a fixme explaining we should raise instead and let's move on. ardumont: Please, add a fixme explaining we should raise instead and let's move on.
So we can actually… | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsOk, thanks. Added a fixme with a link to this very place. borisbaldassari: Ok, thanks.
Added a fixme with a link to this very place. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if m_doc is not None: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
doc_id = int(m_doc.group("doc")) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
self.incremental | |||||||||||||||||||||||||||||||||||||||||||||||||||||
and self.state | |||||||||||||||||||||||||||||||||||||||||||||||||||||
and self.state.last_seen_doc | |||||||||||||||||||||||||||||||||||||||||||||||||||||
and self.state.last_seen_doc >= doc_id | |||||||||||||||||||||||||||||||||||||||||||||||||||||
): | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# jar_src["doc"] contains the id of the current document, whatever | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# its type (scm or jar). | |||||||||||||||||||||||||||||||||||||||||||||||||||||
jar_src["doc"] = None | |||||||||||||||||||||||||||||||||||||||||||||||||||||
else: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
jar_src["doc"] = doc_id | |||||||||||||||||||||||||||||||||||||||||||||||||||||
else: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# If incremental mode, we don't record any line that is | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# before our last recorded doc id. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if self.incremental and jar_src["doc"] is None: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
continue | |||||||||||||||||||||||||||||||||||||||||||||||||||||
m_val = re_val.match(line) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if m_val is not None: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
(gid, aid, version, classifier, ext) = m_val.groups() | |||||||||||||||||||||||||||||||||||||||||||||||||||||
ext = ext.strip() | |||||||||||||||||||||||||||||||||||||||||||||||||||||
path = "/".join(gid.split(".")) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if classifier == "NA" and ext.lower() == "pom": | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# If incremental mode, we don't record any line that is | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# before our last recorded doc id. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
self.incremental | |||||||||||||||||||||||||||||||||||||||||||||||||||||
and self.state | |||||||||||||||||||||||||||||||||||||||||||||||||||||
and self.state.last_seen_pom | |||||||||||||||||||||||||||||||||||||||||||||||||||||
and self.state.last_seen_pom >= doc_id | |||||||||||||||||||||||||||||||||||||||||||||||||||||
): | |||||||||||||||||||||||||||||||||||||||||||||||||||||
continue | |||||||||||||||||||||||||||||||||||||||||||||||||||||
url_path = f"{path}/{aid}/{version}/{aid}-{version}.{ext}" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
url_pom = urljoin(self.BASE_URL, url_path,) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
out_pom[url_pom] = doc_id | |||||||||||||||||||||||||||||||||||||||||||||||||||||
elif ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
classifier.lower() == "sources" or ("src" in classifier) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
) and ext.lower() in ("zip", "jar"): | |||||||||||||||||||||||||||||||||||||||||||||||||||||
url_path = ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
f"{path}/{aid}/{version}/{aid}-{version}-{classifier}.{ext}" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actions
ardumont: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actions
log instruction. ardumont: log instruction. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
url_src = urljoin(self.BASE_URL, url_path) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
jar_src["gid"] = gid | |||||||||||||||||||||||||||||||||||||||||||||||||||||
jar_src["aid"] = aid | |||||||||||||||||||||||||||||||||||||||||||||||||||||
jar_src["version"] = version | |||||||||||||||||||||||||||||||||||||||||||||||||||||
else: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
m_time = re_time.match(line) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if m_time is not None and url_src is not None: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
time = m_time.group("mtime") | |||||||||||||||||||||||||||||||||||||||||||||||||||||
jar_src["time"] = int(time) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
logger.debug(f"* Yielding jar {url_src}.") | |||||||||||||||||||||||||||||||||||||||||||||||||||||
yield { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"type": "maven", | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"url": url_src, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
**jar_src, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
} | |||||||||||||||||||||||||||||||||||||||||||||||||||||
url_src = None | |||||||||||||||||||||||||||||||||||||||||||||||||||||
logger.info(f"Found {len(out_pom)} poms.") | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# Now fetch pom files and scan them for scm info. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
logger.info("Fetching poms..") | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actions
vlorentz: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
for pom in out_pom: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actionsdebug log instruction, please ardumont: debug log instruction, please | |||||||||||||||||||||||||||||||||||||||||||||||||||||
text = self.page_request(pom, {}) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
try: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
project = xmltodict.parse(text.content.decode()) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if "scm" in project["project"]: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if "connection" in project["project"]["scm"]: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsI have a hard time reading the for logic which as far as i can tell is about parsing the index. It would match what's done in the gnu lister (granted it's easier in the gnu lister but still). ardumont: I have a hard time reading the for logic which as far as i can tell is about parsing the index. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsYes, the parsing is a bit tough, but we don't have much choice, see my comment about the origin of this text file. I certainly could change where the yield occurs, but I did it following douardda's comment in https://forge.softwareheritage.org/D6133#inline-45665 I'm ready to change it, though. No personal strong opinion on that. borisbaldassari: Yes, the parsing is a bit tough, but we don't have much choice, see my comment about the origin… | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsWhat if you built a temporary data structure? For example this: doc 0 field 0 name u type string value al.aldi|sprova4j|0.1.0|sources|jar field 1 name m type string value 1626111735737 would be turned into something like this: { "doc 0": { "field 0": { "name": "u", "type": "string", "value": "al.aldi|sprova4j|0.1.0|sources|jar", }, "field 1": { "name": "m", "type": "string", "value": "1626111735737", }, } } This would allow splitting the parser from the rest of the logic vlorentz: What if you built a temporary data structure? For example this:
```
doc 0
field 0
name u… | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsYes, that'd be on the path of my initial suggestion about separating parsing and reading logics. Parsing/format conversion is extracted in some easier code to read and maintain Then the listing focus on the incremental listing (or not) logic without having to parse anything, just Note that the lister speed is not that important as long as it yields origins regularly (then it's updating Note that, for my part, this reading improvment can be taken care of in another diff if that's easier to handle... ardumont: Yes, that'd be on the path of my initial suggestion about separating parsing and reading logics. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsSo. Here is my humble proposal to split the parsing to make it more readable. Unfortunately the proposed data structure doesn't exactly work in practice because:
I tried however to keep the same spirit; please allow me to propose a slightly different intermediate version of the data structure: { 0: { 'type': 'maven', 'url': 'https://repo1.maven.org/maven2/al/aldi/sprova4j/0.1.0/sprova4j-0.1.0-sources.jar', 'time': 1626109619335, 'gid': 'al.aldi', 'aid': 'sprova4j', 'version': '0.1.0' }, 1: { 'type': 'scm', 'url': 'https://repo1.maven.org/maven2/al/aldi/sprova4j/0.1.0/sprova4j-0.1.0.pom' }, 2: { 'type': 'maven', 'url': 'https://repo1.maven.org/maven2/al/aldi/sprova4j/0.1.1/sprova4j-0.1.1-sources.jar', 'time': 1626111425534, 'gid': 'al.aldi', 'aid': 'sprova4j', 'version': '0.1.1' }, 3: { 'type': 'scm', 'url': 'https://repo1.maven.org/maven2/al/aldi/sprova4j/0.1.1/sprova4j-0.1.1.pom' } } The resulting parsing code is substantially simpler. Incremental and uniqueness stuff is moved to the second part. In the second part, since we yield jars and scms as we find them, we do not need any more the 2 counters for the incremental recovery: the last_seen_doc is enough. I did not have to change the tests, excepted for:
So it should be ok. Please let me know what you think. borisbaldassari: So. Here is my humble proposal to split the parsing to make it more readable.
Unfortunately… | |||||||||||||||||||||||||||||||||||||||||||||||||||||
scm = project["project"]["scm"]["connection"] | |||||||||||||||||||||||||||||||||||||||||||||||||||||
gid = project["project"]["groupId"] | |||||||||||||||||||||||||||||||||||||||||||||||||||||
aid = project["project"]["artifactId"] | |||||||||||||||||||||||||||||||||||||||||||||||||||||
yield { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"type": "scm", | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"doc": out_pom[pom], | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"url": scm, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"project": f"{gid}.{aid}", | |||||||||||||||||||||||||||||||||||||||||||||||||||||
} | |||||||||||||||||||||||||||||||||||||||||||||||||||||
else: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
logger.debug(f"No scm.connection in pom {pom}") | |||||||||||||||||||||||||||||||||||||||||||||||||||||
else: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
logger.debug(f"No scm in pom {pom}") | |||||||||||||||||||||||||||||||||||||||||||||||||||||
except xmltodict.expat.ExpatError as error: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
logger.info(f"Could not parse POM {pom} XML: {error}. Next.") | |||||||||||||||||||||||||||||||||||||||||||||||||||||
def get_origins_from_page(self, page: RepoPage) -> Iterator[ListedOrigin]: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"""Convert a page of Maven repositories into a list of ListedOrigins. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
assert self.lister_obj.id is not None | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if page["type"] == "scm": | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# If origin is a scm url: detect scm type and yield. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# Note that the official format is: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# scm:git:git://github.com/openengsb/openengsb-framework.git | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# but many, many projects directly put the repo url, so we have to | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# detect the content to match it properly. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
m_scm = re.match(r"^scm:(?P<type>[^:]+):(?P<url>.*)$", page["url"]) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if m_scm is not None: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
scm_type = m_scm.group("type") | |||||||||||||||||||||||||||||||||||||||||||||||||||||
scm_url = m_scm.group("url") | |||||||||||||||||||||||||||||||||||||||||||||||||||||
origin = ListedOrigin( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
lister_id=self.lister_obj.id, url=scm_url, visit_type=scm_type, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsKeep the same order you did below, first match against maven, else deal with scm: if page["type"] == "maven": ... else: # scm type That's easier to read if you always use the same order in conditionals ardumont: Keep the same order you did below, first match against maven, else deal with scm:
```
if page… | |||||||||||||||||||||||||||||||||||||||||||||||||||||
yield origin | |||||||||||||||||||||||||||||||||||||||||||||||||||||
else: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if page["url"].endswith(".git"): | |||||||||||||||||||||||||||||||||||||||||||||||||||||
origin = ListedOrigin( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
lister_id=self.lister_obj.id, url=page["url"], visit_type="git", | |||||||||||||||||||||||||||||||||||||||||||||||||||||
) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
yield origin | |||||||||||||||||||||||||||||||||||||||||||||||||||||
else: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# Origin is a source archive: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
origin = ListedOrigin( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
lister_id=self.lister_obj.id, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
url=page["url"], | |||||||||||||||||||||||||||||||||||||||||||||||||||||
visit_type=page["type"], | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actions
if unused, drop it? ardumont: if unused, drop it? | |||||||||||||||||||||||||||||||||||||||||||||||||||||
extra_loader_arguments={ | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"artifacts": [ | |||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"time": page["time"], | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"gid": page["gid"], | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"aid": page["aid"], | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"version": page["version"], | |||||||||||||||||||||||||||||||||||||||||||||||||||||
} | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actions
ardumont: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
] | |||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | |||||||||||||||||||||||||||||||||||||||||||||||||||||
) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
yield origin | |||||||||||||||||||||||||||||||||||||||||||||||||||||
def commit_page(self, page: RepoPage) -> None: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"""Update currently stored state using the latest listed doc. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Note: this is a noop for full listing mode | |||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if self.incremental and self.state: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# We need to differentiate the two state counters according | |||||||||||||||||||||||||||||||||||||||||||||||||||||
# to the type of origin. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if page["type"] == "maven" and page["doc"] > self.state.last_seen_doc: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
self.state.last_seen_doc = page["doc"] | |||||||||||||||||||||||||||||||||||||||||||||||||||||
elif page["type"] == "scm" and page["doc"] > self.state.last_seen_pom: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
self.state.last_seen_doc = page["doc"] | |||||||||||||||||||||||||||||||||||||||||||||||||||||
self.state.last_seen_pom = page["doc"] | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actionslog ;) ardumont: log ;) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actionshum.. yeah, ok, I got it, sorry.. ;-) borisbaldassari: hum.. yeah, ok, I got it, sorry.. ;-) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
def finalize(self) -> None: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
"""Finalize the lister state, set update if any progress has been made. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Note: this is a noop for full listing mode | |||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if self.incremental and self.state: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
last_seen_doc = self.state.last_seen_doc | |||||||||||||||||||||||||||||||||||||||||||||||||||||
last_seen_pom = self.state.last_seen_pom | |||||||||||||||||||||||||||||||||||||||||||||||||||||
scheduler_state = self.get_state_from_scheduler() | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if last_seen_doc and last_seen_pom: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
if (scheduler_state.last_seen_doc < last_seen_doc) or ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
scheduler_state.last_seen_pom < last_seen_pom | |||||||||||||||||||||||||||||||||||||||||||||||||||||
): | |||||||||||||||||||||||||||||||||||||||||||||||||||||
self.updated = True | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsIs it possible to retrieve the last_update as well [1]? What's the timestamp present in the output of the indexer? [1] The last_update is actually used for scheduling purposes. ardumont: Is it possible to retrieve the `last_update` as well [1]?
What's the timestamp present in the… | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsThe timestamp is indeed the last update time of the artefact, see the Maven indexer documentation for this release. So I guess it can be used for that purpose, yes. I'll add the parameter (with some parsing) and submit the fix for all 3 following comments too. Thanks. borisbaldassari: The timestamp is indeed the last update time of the artefact, see [the Maven indexer… | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actionssame question about last_update (see previous point) ardumont: same question about `last_update` (see previous point) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actionssame question about last_update (see previous point) ardumont: same question about last_update (see previous point) | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actionscan't we make it the same value as last_update in string so as to avoid to have to do the same computation as line 358 in the loader? ardumont: can't we make it the same value as last_update in string so as to avoid to have to do the same… | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actions
Optional[datetime] is Union[None, datetime] but in shorter form ;) Also to fix the current build failure [1], you need to ensure page.time is not None, hence the suggested assert. [1] 11:12:55 swh/lister/maven/lister.py:360: error: Argument 1 to "parse_date" has incompatible type "Optional[str]"; expected "str" ardumont: Optional[datetime] is Union[None, datetime] but in shorter form ;)
Also to fix the current… |
Can you define this type better? A dataclass would be perfect.