build providers: better injection logic #2196

sergiusens · 2018-08-07T22:25:21Z

Have you followed the guidelines for contributing?
Have you signed the CLA?
If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
If this is a new feature. Have you discussed the design on the forum?
Have you successfully run ./runtests.sh static?
Have you successfully run ./runtests.sh unit?

kyrofa · 2018-08-08T17:43:22Z

Conflicts here.

codecov-io · 2018-08-09T11:51:34Z

Codecov Report

Merging #2196 into master will decrease coverage by 0.05%.
The diff coverage is 91.85%.

@@            Coverage Diff             @@
##           master    #2196      +/-   ##
==========================================
- Coverage   91.21%   91.15%   -0.06%     
==========================================
  Files         208      209       +1     
  Lines       13160    13302     +142     
  Branches     1957     1981      +24     
==========================================
+ Hits        12004    12126     +122     
- Misses        788      801      +13     
- Partials      368      375       +7

Impacted Files	Coverage Δ
snapcraft/internal/lifecycle/_containers.py	`82.92% <0%> (ø)`	⬆️
snapcraft/internal/repo/snaps.py	`91.01% <100%> (-0.27%)`	⬇️
...l/build_providers/_multipass/_multipass_command.py	`85.52% <33.33%> (-3.52%)`	⬇️
snapcraft/storeapi/_snap_index_client.py	`77.77% <66.66%> (-1.3%)`	⬇️
.../internal/build_providers/_multipass/_multipass.py	`83.67% <75%> (-6.33%)`	⬇️
snapcraft/internal/build_providers/_snap.py	`94.28% <94.28%> (ø)`
...apcraft/internal/build_providers/_base_provider.py	`98.33% <94.73%> (-1.67%)`	⬇️
snapcraft/internal/pluginhandler/_plugin_loader.py	`91.96% <0%> (-1.79%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b772997...0b9c8c8. Read the comment docs.

Keep a registry of what has been injected and allow for ephemeral injection for disposable environments. LP: #1779945 LP: #1782777 Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>

kyrofa

Looking good! Took a first pass, made some comments inline.

kyrofa · 2018-08-09T17:20:15Z

snapcraft/internal/build_providers/_base_provider.py

@@ -53,6 +47,9 @@ def __init__(self, *, project, echoer) -> None:
            self.snap_filename = "{}_{}.snap".format(
                project.info.name, project.deb_arch
            )
+        self.provider_project_dir = os.path.join(
+            BaseDirectory.xdg_data_home, "snapcraft", "projects", project.info.name


Perhaps os.path.join(xdg.BaseDirectory.save_data_path("snapcraft"), "projects", project.info.name) ?

Sure, why not? Is there a difference?

kyrofa · 2018-08-09T17:32:16Z

snapcraft/internal/build_providers/_snap.py

+        if not snap_repo.installed and self._latest_revision is None:
+            op = _SnapOp.INSTALL
+        elif not snap_repo.installed and self._latest_revision is not None:
+            op = _SnapOp.REFRESH


I'm a little confused on this one-- how can a refresh be what's needed if it's not installed?

If you are running from the deb, it will not be installed, if the environment has already been created it may have a revision installed.
snap_repo is for the host, the operation for the build environment.

I will document with code comments

kyrofa · 2018-08-09T17:33:58Z

snapcraft/internal/build_providers/_snap.py

+]
+
+
+class SnapInjector:


Mind adding some sphinx docs here?

kyrofa · 2018-08-09T17:34:19Z

snapcraft/internal/build_providers/_snap.py

+        runner,
+        snap_dir_mounter,
+        snap_dir_unmounter,
+        file_pusher


Would like to see some types specified for these callbacks.

yes, I forgot!

there is no comprehensive way to define a Callable that takes optional arguments nor one that takes kwargs https://docs.python.org/3.5/library/typing.html#callable

kyrofa · 2018-08-09T17:43:49Z

snapcraft/internal/build_providers/_snap.py

+    def _get_latest_revision(self, snap_name):
+        self._load_registry()
+        try:
+            return self._registry_data[snap_name][-1]["revision"]


Rather than having an explicit _load_registry() function as well as a private _registry_data attribute, this usage seems better suited to simple lazy evaluation of the property (or a get_registry() function, if you prefer) to encapsulate the loading and returning of the registry.

I sort of started to dislike properties when I do not need them (like replacing an already in use attribute). They are hard to mock and this clearly does something under the hood. Also,

Explicit is better than implicit" is in direct conflict with using a property. (It looks like a simple assignment, but it calls a function).

After that philosophical point, if you still want me to change, I will :-)

Oh, but I am down with get_registry, I had that originally and was passing around too many params for my taste, but I prefer that indeed

I like the symmetry on this one and not sure I understand the change now that I am re-reading.

kyrofa · 2018-08-09T17:47:46Z

snapcraft/internal/build_providers/_snap.py

+        with open(self._registry_filepath) as registry_file:
+            self._registry_data = yaml.load(registry_file)
+
+    def _dump_registry(self):


Take it or leave it, but I feel like this should be called _save_registry.

ok, was going with parity with yaml and json.load/dump :-)

kyrofa · 2018-08-09T17:50:32Z

snapcraft/internal/build_providers/_snap.py

+        except (IndexError, KeyError):
+            return None
+
+    def _add_latest_revision(self, snap_name: str, snap_revision: str) -> None:


This function name isn't a good indicator of what this does. It's not at all similar to the similarly-named add() function, and it doesn't do anything with the "latest revision": it takes the one you give it. Perhaps _record_revision() would be better?

sure, this was mostly a parity thing, but I even got that wrong I think :-)

into snap-processor

kyrofa

I have two nits, but this looks good to me.

kyrofa · 2018-08-14T18:08:50Z

snapcraft/internal/build_providers/_snap.py

+        return self.__install_cmd
+
+
+_STORE_ASSERTION = [


Can we move this up to the top of the file, please?

kyrofa · 2018-08-14T18:11:34Z

snapcraft/internal/build_providers/_snap.py

+                            environment.
+        """
+
+        self._snaps = []  # type: List["_SnapManager"]


Does this type need to be quoted? It's defined earlier in this file.

only because I moved things around, probably not

into snap-processor

sergiusens force-pushed the snap-processor branch from 68506c1 to fbd1b72 Compare August 7, 2018 22:27

sergiusens force-pushed the snap-processor branch from fbd1b72 to e9fd09e Compare August 9, 2018 11:51

build providers: better injection logic

a8b0624

Keep a registry of what has been injected and allow for ephemeral injection for disposable environments. LP: #1779945 LP: #1782777 Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>

sergiusens force-pushed the snap-processor branch from e9fd09e to a8b0624 Compare August 9, 2018 11:54

sergiusens changed the title ~~Snap processor~~ build providers: better injection logic Aug 9, 2018

kyrofa reviewed Aug 9, 2018

View reviewed changes

sergiusens added 13 commits August 10, 2018 08:25

use save_data_path instead of xdg_data_home

d1e6125

clarify where snap_repo operates

77f3365

docstrings

ddb0602

typing and typing

f5a1cc6

save registry

6ea6aac

more typing

10646a1

Merge remote-tracking branch 'origin/master' into snap-processor

acd331d

Merge branch 'master' into snap-processor

01536ec

support more OS scenarios

d86f461

Merge remote-tracking branch 'origin/master' into snap-processor

a2faea6

registry

5cd985e

Merge branch 'snap-processor' of https://github.com/sergiusens/snapcraft

1e2b80c

into snap-processor

Merge branch 'master' into snap-processor

e633adb

kyrofa approved these changes Aug 14, 2018

View reviewed changes

sergiusens added 3 commits August 14, 2018 15:40

move definition

c689921

unquote static

66a2ce7

Merge branch 'snap-processor' of https://github.com/sergiusens/snapcraft

0b9c8c8

into snap-processor

sergiusens merged commit d9a477e into canonical:master Aug 14, 2018

sergiusens deleted the snap-processor branch August 14, 2018 23:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build providers: better injection logic #2196

build providers: better injection logic #2196

sergiusens commented Aug 7, 2018 •

edited

kyrofa commented Aug 8, 2018

codecov-io commented Aug 9, 2018 •

edited

kyrofa left a comment

kyrofa Aug 9, 2018

sergiusens Aug 9, 2018

kyrofa Aug 9, 2018

sergiusens Aug 9, 2018

sergiusens Aug 9, 2018

kyrofa Aug 9, 2018

sergiusens Aug 10, 2018

kyrofa Aug 9, 2018

sergiusens Aug 9, 2018

sergiusens Aug 10, 2018

kyrofa Aug 9, 2018

sergiusens Aug 9, 2018

sergiusens Aug 9, 2018

sergiusens Aug 10, 2018

kyrofa Aug 9, 2018

sergiusens Aug 9, 2018

kyrofa Aug 9, 2018

sergiusens Aug 9, 2018

kyrofa left a comment

kyrofa Aug 14, 2018

sergiusens Aug 14, 2018

kyrofa Aug 14, 2018

sergiusens Aug 14, 2018

		]


		class SnapInjector:

build providers: better injection logic #2196

build providers: better injection logic #2196

Conversation

sergiusens commented Aug 7, 2018 • edited

kyrofa commented Aug 8, 2018

codecov-io commented Aug 9, 2018 • edited

Codecov Report

kyrofa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiusens commented Aug 7, 2018 •

edited

codecov-io commented Aug 9, 2018 •

edited