← Back to team overview

launchpad-reviewers team mailing list archive

Re: [Merge] ~pelpsi/launchpad-buildd:fetch-service-apt-initialization into launchpad-buildd:master

 


Diff comments:

> diff --git a/lpbuildd/target/build_rock.py b/lpbuildd/target/build_rock.py
> index 2da488c..6eeb312 100644
> --- a/lpbuildd/target/build_rock.py
> +++ b/lpbuildd/target/build_rock.py
> @@ -76,7 +87,14 @@ class BuildRock(
>                  "python3-setuptools",
>              ]
>          )
> -        self.backend.run(["apt-get", "-y", "install"] + deps)
> +        if env:

Maybe we can re-write this if else loop as 

self.backend.run(["apt-get", "-y", "install"] + deps, env=env if env else None)

> +            self.backend.run(["apt-get", "-y", "install"] + deps, env=env)
> +        else:
> +            self.backend.run(["apt-get", "-y", "install"] + deps)
> +        if self.args.use_fetch_service:
> +            self.install_snapd_proxy(proxy_url=self.args.proxy_url)
> +            self.restart_snapd()
> +            self.configure_git_protocol_v2()
>          if self.backend.supports_snapd:
>              self.snap_store_set_proxy()
>          for snap_name, channel in sorted(self.args.channels.items()):
> @@ -84,31 +102,55 @@ class BuildRock(
>              # which disables all sandboxing to ensure it runs with no strict
>              # confinement.
>              if snap_name != "rockcraft":
> +                if env:

Similarly, this can be re-written to reduce duplicate code.

> +                    self.backend.run(
> +                        [
> +                            "snap",
> +                            "install",
> +                            "--channel=%s" % channel,
> +                            snap_name
> +                        ],
> +                        env=env
> +                    )
> +                else:
> +                    self.backend.run(
> +                        [
> +                            "snap",
> +                            "install",
> +                            "--channel=%s" % channel,
> +                            snap_name
> +                        ]
> +                    )
> +        if "rockcraft" in self.args.channels:

Similarly for all the following if else clauses that checks only for env.

> +            if env:
>                  self.backend.run(
> -                    ["snap", "install", "--channel=%s" % channel, snap_name]
> +                    [
> +                        "snap",
> +                        "install",
> +                        "--classic",
> +                        "--channel=%s" % self.args.channels["rockcraft"],
> +                        "rockcraft",
> +                    ],
> +                    env=env
> +                )
> +            else:
> +                self.backend.run(
> +                    [
> +                        "snap",
> +                        "install",
> +                        "--classic",
> +                        "--channel=%s" % self.args.channels["rockcraft"],
> +                        "rockcraft",
> +                    ]
>                  )
> -        if "rockcraft" in self.args.channels:
> -            self.backend.run(
> -                [
> -                    "snap",
> -                    "install",
> -                    "--classic",
> -                    "--channel=%s" % self.args.channels["rockcraft"],
> -                    "rockcraft",
> -                ]
> -            )
>          else:
> -            self.backend.run(["snap", "install", "--classic", "rockcraft"])
> -
> -        if self.args.use_fetch_service:
> -            # Deleting apt cache /var/lib/apt/lists before
> -            # installing the fetch service
> -            self.delete_apt_cache()
> -            self.install_mitm_certificate()
> -            self.install_snapd_proxy(proxy_url=self.args.proxy_url)
> -            self.backend.run(["apt-get", "-y", "update"])
> -            self.restart_snapd()
> -            self.configure_git_protocol_v2()
> +            if env:
> +                self.backend.run(
> +                    ["snap", "install", "--classic", "rockcraft"],
> +                    env=env
> +                )
> +            else:
> +                self.backend.run(["snap", "install", "--classic", "rockcraft"])
>  
>          # With classic confinement, the snap can access the whole system.
>          # We could build the rock in /build, but we are using /home/buildd


-- 
https://code.launchpad.net/~pelpsi/launchpad-buildd/+git/launchpad-buildd/+merge/473926
Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad-buildd:fetch-service-apt-initialization into launchpad-buildd:master.



References