launchpad-reviewers team mailing list archive

[Andrey Fedoseev <mp+426425@xxxxxxxxxxxxxxxxxx>]

I left a comment

Diff comments:

> diff --git a/lib/lp/soyuz/model/archivejob.py b/lib/lp/soyuz/model/archivejob.py
> index ab98138..65a62af 100644
> --- a/lib/lp/soyuz/model/archivejob.py
> +++ b/lib/lp/soyuz/model/archivejob.py
> @@ -330,57 +349,60 @@ class CIBuildUploadJob(ArchiveJobDerived):
>              "user_defined_fields": [("subdir", index["subdir"])],
>              }
>  
> -    def _scanFile(self, path):
> -        # XXX cjwatson 2022-06-10: We should probably start splitting this
> -        # up some more.
> -        name = os.path.basename(path)
> -        if path.endswith(".whl"):
> -            try:
> -                parsed_path = parse_wheel_filename(path)
> -                wheel = Wheel(path)
> -            except Exception as e:
> -                raise ScanException("Failed to scan %s" % name) from e
> -            return {
> -                "name": wheel.name,
> -                "version": wheel.version,
> -                "summary": wheel.summary or "",
> -                "description": wheel.description,
> -                "binpackageformat": BinaryPackageFormat.WHL,
> -                "architecturespecific": "any" not in parsed_path.platform_tags,
> -                "homepage": wheel.home_page or "",
> -                }
> -        elif path.endswith(".tar.bz2"):
> -            try:
> -                with tarfile.open(path) as tar:
> +    def _scanCondaV1(self, path):
> +        try:
> +            with tarfile.open(path) as tar:
> +                index = json.loads(
> +                    tar.extractfile("info/index.json").read().decode())
> +                about = json.loads(
> +                    tar.extractfile("info/about.json").read().decode())
> +        except Exception as e:
> +            logger.warning(
> +                "Failed to scan %s as a Conda v1 package: %s",
> +                os.path.basename(path), e)
> +            return None
> +        scanned = {"binpackageformat": BinaryPackageFormat.CONDA_V1}
> +        scanned.update(self._scanCondaMetadata(index, about))
> +        return scanned
> +
> +    def _scanCondaV2(self, path):
> +        try:
> +            with zipfile.ZipFile(path) as zipf:
> +                base_name = os.path.basename(path)[:-len(".conda")]
> +                info = io.BytesIO()
> +                with zipf.open("info-%s.tar.zst" % base_name) as raw_info:
> +                    zstandard.ZstdDecompressor().copy_stream(raw_info, info)
> +                info.seek(0)
> +                with tarfile.open(fileobj=info) as tar:
>                      index = json.loads(
>                          tar.extractfile("info/index.json").read().decode())
>                      about = json.loads(
>                          tar.extractfile("info/about.json").read().decode())
> -            except Exception as e:
> -                raise ScanException("Failed to scan %s" % name) from e
> -            scanned = {"binpackageformat": BinaryPackageFormat.CONDA_V1}
> -            scanned.update(self._scanCondaMetadata(index, about))
> -            return scanned
> -        elif path.endswith(".conda"):
> -            try:
> -                with zipfile.ZipFile(path) as zipf:
> -                    base_name = os.path.basename(path)[:-len(".conda")]
> -                    info = io.BytesIO()
> -                    with zipf.open("info-%s.tar.zst" % base_name) as raw_info:
> -                        zstandard.ZstdDecompressor().copy_stream(
> -                            raw_info, info)
> -                    info.seek(0)
> -                    with tarfile.open(fileobj=info) as tar:
> -                        index = json.loads(
> -                            tar.extractfile("info/index.json").read().decode())
> -                        about = json.loads(
> -                            tar.extractfile("info/about.json").read().decode())
> -            except Exception as e:
> -                raise ScanException("Failed to scan %s" % name) from e
> -            scanned = {"binpackageformat": BinaryPackageFormat.CONDA_V2}
> -            scanned.update(self._scanCondaMetadata(index, about))
> -            return scanned
> +        except Exception as e:
> +            logger.warning(
> +                "Failed to scan %s as a Conda v2 package: %s",
> +                os.path.basename(path), e)
> +            return None
> +        scanned = {"binpackageformat": BinaryPackageFormat.CONDA_V2}
> +        scanned.update(self._scanCondaMetadata(index, about))
> +        return scanned
> +
> +    def _scanFile(self, path):

This may be a matter of personal preference, but I'd do it like so:

```
for suffix, scanner in _scanners:
    if path.endswith(suffix):
       break
else:
    logger.info("No upload handler for %s", os.path.basename(path))
    return

return scanner(path)
```


Also, in the description you say:

> we now keep trying other scanners even if one fails, which will be necessary in the long run since some package types have ambiguous file names.

To me it looks like we try only one scanner, the one that matches the file name.

> +        _scanners = (
> +            (".whl", self._scanWheel),
> +            (".tar.bz2", self._scanCondaV1),
> +            (".conda", self._scanCondaV2),
> +            )
> +        found_scanner = False
> +        for suffix, scanner in _scanners:
> +            if path.endswith(suffix):
> +                found_scanner = True
> +                scanned = scanner(path)
> +                if scanned is not None:
> +                    return scanned
>          else:
> +            if not found_scanner:
> +                logger.info("No upload handler for %s", os.path.basename(path))
>              return None
>  
>      def _scanArtifacts(self, artifacts):


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/426425
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:refactor-ci-build-upload-job-scan-file into launchpad:master.