launchpad-reviewers team mailing list archive

[Simone Pelosi <mp+461063@xxxxxxxxxxxxxxxxxx>]

Thank you for your precious reviews, I firstly addressed some of your comments here, and I'm working to remove snapcraft.yaml logic to search for components as suggested by William. I will use the build files to retrieve them. 

I also had a chat with Przemysław, and he clarified that snap_name cannot contain `+` nor `_`, the same as component_name. So it should be easier to extract the component name in that context `<snap_name>+<component_name>_<component_revision>.comp`.

Diff comments:

> diff --git a/lib/lp/snappy/model/snapbuildjob.py b/lib/lp/snappy/model/snapbuildjob.py
> index 539fb1c..80260c1 100644
> --- a/lib/lp/snappy/model/snapbuildjob.py
> +++ b/lib/lp/snappy/model/snapbuildjob.py
> @@ -333,6 +351,24 @@ class SnapStoreUploadJob(SnapBuildJobDerived):
>                  pass
>          return timedelta(minutes=1)
>  
> +    @staticmethod
> +    def _extract_component_name(filename, snapcraft_components):
> +        """Extract component name from filename
> +
> +        Use the snapcraft_components keys to extract the component name
> +        from the built component filename. Each key correspond to
> +        <component_name> while the built filename has the following pattern:
> +        <snap_name>+<component_name>_<component_revision>.comp.
> +        """
> +        for component in snapcraft_components:
> +            # Surround component name with `+` and `_` to avoid
> +            # superset of the same component name.
> +            # ie. testcomp -> testcomponent
> +            built_component = "+%s_" % component

This chance doesn't exist since we are surrounding the comp name with `+` and `_` before checking. But I have to change this function and revert it to the previous behavior because as said by William snapcraft.yaml is not reliable.

> +            if built_component in filename:
> +                return component
> +        return filename
> +
>      def run(self):
>          """See `IRunnableJob`."""
>          client = getUtility(ISnapStoreClient)
> @@ -350,16 +386,54 @@ class SnapStoreUploadJob(SnapBuildJobDerived):
>                      # Nothing to do.
>                      self.error_message = None
>                      return
> +
> +                components_ids = {}
> +                components = []
> +                try:
> +                    # Get components from snapcraft.yaml
> +                    snapcraft_yaml = getUtility(ISnapSet).getSnapcraftYaml(
> +                        self.snapbuild.snap
> +                    )
> +                    if snapcraft_yaml["components"]:
> +                        for component in snapcraft_yaml["components"]:
> +                            components_ids[component] = None
> +                    # Get built components from built files
> +                    for _, lfa, _ in self.snapbuild.getFiles():
> +                        if lfa.filename.endswith(".comp"):
> +                            components.append(lfa)
> +                except (
> +                    MissingSnapcraftYaml,
> +                    CannotFetchSnapcraftYaml,
> +                    CannotParseSnapcraftYaml,
> +                ):
> +                    pass
> +
> +                if "components_ids" not in self.store_metadata:
> +                    self.components_ids = components_ids
>                  if "upload_id" not in self.store_metadata:
>                      self.upload_id = client.uploadFile(snap_lfa)
>                      # We made progress, so reset attempt_count.
>                      self.attempt_count = 1
> +                # Process components
> +                for component in components:
> +                    # if the id is None, we need to upload the component
> +                    # because it means that that component was never uploaded.
> +                    # Note that the id is returned directly from SnapStore API.
> +                    comp_name = self._extract_component_name(
> +                        component.filename, components_ids
> +                    )
> +                    if self.components_ids.get(comp_name) == None:
> +                        self.components_ids[comp_name] = client.uploadFile(
> +                            component
> +                        )

Yep, `test_run_502_retries_with_components` tests the mentioned behavior: note that we are committing when we are handling an Exception:
```
        except Exception:
            # The normal job infrastructure will abort the transaction, but
            # we want to commit instead: the only database changes we make
            # are to this job's metadata and should be preserved.
            transaction.commit()
            raise
```

> +                        self.attempt_count = 1
>                  if "status_url" not in self.store_metadata:
>                      self.status_url = client.push(
> -                        self.snapbuild, self.upload_id
> +                        self.snapbuild, self.upload_id, self.components_ids
>                      )
>                      # We made progress, so reset attempt_count.
>                      self.attempt_count = 1
> +
>                  if self.store_url is None:
>                      # This is no longer strictly necessary as the store is
>                      # handling releases via the release intent, but we


-- 
https://code.launchpad.net/~pelpsi/launchpad/+git/launchpad/+merge/461063
Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad:snap-component-integration into launchpad:master.