many: automatically detect dependency changes #2165

kyrofa · 2018-06-20T14:29:51Z

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?

Currently the only way parts know they're dirty from dependency changes is if the lifecycle tells them so. This is a very error-prone approach, since if the lifecycle forgets to tell the part, then it doesn't know it needs to be cleaned. This PR fixes LP: #1778081 by automatically detecting changes instead of noting them in code, which also simplifies the upcoming feature of detecting source changes.

Note that this PR breaks backward state compatibility with #2152 (i.e. running on trees made dirty since then will break). This was judged to be acceptable based on the fact that it has not yet been contained in a release.

Currently the only way parts know they're dirty from dependency changes is if the lifecycle tells them so. This is a very error-prone approach, since if the lifecycle forgets to tell the part, then it doesn't know it needs to be cleaned. Dependency changes should be automatically detected instead of being noted in code, which also simplifies the upcoming feature of detecting source changes. Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

codecov-io · 2018-06-20T20:00:49Z

Codecov Report

Merging #2165 into master will increase coverage by 0.04%.
The diff coverage is 97.86%.

@@            Coverage Diff             @@
##           master    #2165      +/-   ##
==========================================
+ Coverage   91.21%   91.26%   +0.04%     
==========================================
  Files         198      200       +2     
  Lines       12471    12560      +89     
  Branches     1846     1869      +23     
==========================================
+ Hits        11376    11463      +87     
- Misses        741      742       +1     
- Partials      354      355       +1

Impacted Files	Coverage Δ
snapcraft/internal/states/_build_state.py	`94.44% <100%> (ø)`	⬆️
snapcraft/internal/states/_state.py	`88.23% <100%> (+3.05%)`	⬆️
snapcraft/internal/errors.py	`99.53% <100%> (-0.02%)`	⬇️
snapcraft/internal/states/_prime_state.py	`91.3% <100%> (ø)`	⬆️
snapcraft/internal/states/_pull_state.py	`93.75% <100%> (ø)`	⬆️
snapcraft/internal/pluginhandler/__init__.py	`92.29% <100%> (-0.13%)`	⬇️
snapcraft/internal/lifecycle/_status_cache.py	`100% <100%> (ø)`
snapcraft/internal/lifecycle/_clean.py	`85.21% <100%> (+0.12%)`	⬆️
snapcraft/internal/lifecycle/__init__.py	`100% <100%> (ø)`	⬆️
snapcraft/internal/lifecycle/_runner.py	`92.05% <100%> (-0.81%)`	⬇️
... and 6 more

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 efebd07...33cd1db. Read the comment docs.

kyrofa · 2018-06-21T21:26:09Z

snapcraft/internal/lifecycle/__init__.py

@@ -19,3 +19,4 @@
 from ._init import init                   # noqa
 from ._packer import pack                 # noqa
 from ._runner import execute              # noqa
+from ._status_cache import StatusCache    # noqa


This probably doesn't need to be exported anymore.

kyrofa · 2018-06-21T21:34:01Z

snapcraft/internal/lifecycle/_runner.py

@@ -299,36 +277,22 @@ def _run_step(self, *, step: steps.Step, part, progress, hint='',

        notify_part_progress(part, progress, hint)
        getattr(part, step.name)()
-        self._steps_run[part.name].add(step)
+        self._cache.add_step_run(part, step)


Add comments here

kyrofa · 2018-06-21T21:38:02Z

snapcraft/internal/pluginhandler/_dirty_report.py

+        self.dirty_project_options = dirty_project_options
+        self.changed_dependencies = changed_dependencies
+
+    def report(self):


Make these get_*

kyrofa · 2018-06-21T21:40:23Z

snapcraft/internal/lifecycle/_status_cache.py

+        """
+        self.config = config
+        self._steps_run = dict()  # type: Dict[str, Set[steps.Step]]
+        self._dirty_reports = collections.defaultdict(dict)  # type: Dict[str, Dict[steps.Step, pluginhandler.DirtyReport]]  # noqa


Create type for this

kyrofa · 2018-06-21T21:43:41Z

snapcraft/internal/lifecycle/_status_cache.py

+        self._ensure_steps_run(part)
+        self._steps_run[part.name].add(step)
+
+    def step_has_run(self, part: pluginhandler.PluginHandler,


has_step_run

sergiusens · 2018-06-21T22:21:18Z

snapcraft/internal/lifecycle/_runner.py

        common.reset_env()
-        prereqs = self.parts_config.get_prereqs(part.name)
+        prereq_parts = self.parts_config.get_dependencies(part.name)


so we had a nice conversation about get_dependencies being renamed, but the local var is still called prereq_parts?

Yeah good catch, fixed.

sergiusens · 2018-06-21T22:22:27Z

snapcraft/internal/lifecycle/_runner.py

-        step_prereqs = {p for p in prereqs
-                        if prerequisite_step not in self._steps_run[p]}
+        prerequisite_step = steps.get_dependency_prerequisite_step(step)
+        step_prereqs = {p for p in prereq_parts


Fixed as well.

sergiusens · 2018-06-21T22:22:32Z

snapcraft/internal/lifecycle/_runner.py


        if step_prereqs:
+            prereq_names = {p.name for p in step_prereqs}


sergiusens · 2018-06-21T22:24:17Z

snapcraft/internal/lifecycle/_status_cache.py

+            2. Is dirty
+            3. Either (1) or (2) apply to any earlier steps in its lifecycle
+        """
+        if (not self.step_has_run(part, step) or


in this use I can see why this may be nice to have in the pluginhandler itself.

sergiusens · 2018-06-21T22:26:24Z

snapcraft/internal/lifecycle/_status_cache.py

+        self._steps_run = dict()  # type: Dict[str, Set[steps.Step]]
+        self._dirty_reports = collections.defaultdict(dict)  # type: Dict[str, Dict[steps.Step, pluginhandler.DirtyReport]]  # noqa
+
+    def step_should_run(self, part: pluginhandler.PluginHandler,


iif we follow the pattern, should_step_run

sergiusens · 2018-06-21T22:32:16Z

snapcraft/internal/lifecycle/_status_cache.py

+                                prerequisite_step)
+                        except errors.StepHasNotRunError:
+                            dependency_changed = True
+                        else:


I always have to lookup if else is for the try succeeding or no matching except 🤷‍♂️

sergiusens · 2018-06-21T22:36:50Z

snapcraft/internal/lifecycle/_status_cache.py

+
+    def _ensure_dirty_report(self, part: pluginhandler.PluginHandler,
+                             step: steps.Step) -> None:
+        if step not in self._dirty_reports[part.name]:


how about returning early if it is in and bumping everything below one level in?
It would make it obvious that there is no else case below

fwiw, this is the only scenario where I am mostly ok with multiple return scenarios, i.e.; prechecks.

Done, actually was able to bring it back two stops.

sergiusens · 2018-06-21T22:38:37Z

snapcraft/internal/pluginhandler/__init__.py


        return None

    def should_step_run(self, step, force=False):
        return force or self.is_clean(step)

+    def step_timestamp(self, step):
+        try:
+            return os.stat(


I can tell you like stairs :taunt: (emoji request 😄 )

Yikes, fixed that a bit.

sergiusens · 2018-06-21T22:40:14Z

snapcraft/internal/pluginhandler/_dirty_report.py

+    def summary(self):
+        reasons = []
+
+        reason_count = 0


reasons_count?

Sure, done.

sergiusens · 2018-06-21T22:43:38Z

snapcraft/internal/pluginhandler/_dirty_report.py

+
+
+class DirtyReport:
+    def __init__(self, *, dirty_properties=None, dirty_project_options=None,


types please

Ah yes of course, fixed.

sergiusens · 2018-06-21T22:48:51Z

snapcraft/internal/pluginhandler/_dirty_report.py

+                reasons.append('properties')
+            else:
+                reasons.append('{!r} property'.format(
+                    next(iter(self.dirty_properties))))


This confuses me and I couldn't fine a unit test; so given dirty_properties={'foo', 'bar'} (as in some test code below, this would reasons.append only foo? Is that the objective or am I missing something here?

Nah, this branch is only hit if there is a single property. This is just getting at the property in a manner that works for both lists and sets. You're absolutely right though, I'm missing tests for these, so I'll add those which should clarify further.

LP: #1778081 Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

kyrofa · 2018-06-23T00:36:45Z

It seems rust is failing for every PR. Ignoring and merging.

kyrofa force-pushed the feature/stop_marking_dirty branch 2 times, most recently from b9b5fc2 to a137e67 Compare June 20, 2018 15:57

kyrofa force-pushed the feature/stop_marking_dirty branch from a137e67 to 9e66ac0 Compare June 20, 2018 17:28

kyrofa commented Jun 21, 2018

View reviewed changes

sergiusens reviewed Jun 21, 2018

View reviewed changes

kyrofa force-pushed the feature/stop_marking_dirty branch 5 times, most recently from 498587a to 3a5e97e Compare June 22, 2018 18:41

Review response

33cd1db

LP: #1778081 Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

kyrofa force-pushed the feature/stop_marking_dirty branch from 3a5e97e to 33cd1db Compare June 22, 2018 19:50

kyrofa merged commit 13881c3 into canonical:master Jun 23, 2018

kyrofa deleted the feature/stop_marking_dirty branch June 23, 2018 00:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: automatically detect dependency changes #2165

many: automatically detect dependency changes #2165

kyrofa commented Jun 20, 2018 •

edited

codecov-io commented Jun 20, 2018 •

edited

kyrofa Jun 21, 2018

kyrofa Jun 22, 2018

kyrofa Jun 21, 2018

kyrofa Jun 22, 2018

kyrofa Jun 21, 2018

kyrofa Jun 22, 2018

kyrofa Jun 21, 2018

kyrofa Jun 22, 2018

kyrofa Jun 21, 2018

kyrofa Jun 22, 2018

sergiusens Jun 21, 2018

kyrofa Jun 22, 2018

sergiusens Jun 21, 2018

kyrofa Jun 22, 2018

sergiusens Jun 21, 2018

kyrofa Jun 22, 2018

sergiusens Jun 21, 2018

sergiusens Jun 21, 2018

sergiusens Jun 21, 2018

sergiusens Jun 21, 2018

sergiusens Jun 21, 2018

kyrofa Jun 22, 2018

sergiusens Jun 21, 2018

kyrofa Jun 22, 2018

sergiusens Jun 21, 2018

kyrofa Jun 22, 2018

sergiusens Jun 21, 2018

kyrofa Jun 22, 2018

sergiusens Jun 21, 2018

kyrofa Jun 21, 2018

kyrofa Jun 22, 2018

kyrofa commented Jun 23, 2018


		if step_prereqs:
		prereq_names = {p.name for p in step_prereqs}



		class DirtyReport:
		def __init__(self, *, dirty_properties=None, dirty_project_options=None,

many: automatically detect dependency changes #2165

many: automatically detect dependency changes #2165

Conversation

kyrofa commented Jun 20, 2018 • edited

codecov-io commented Jun 20, 2018 • edited

Codecov Report

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

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 commented Jun 23, 2018

kyrofa commented Jun 20, 2018 •

edited

codecov-io commented Jun 20, 2018 •

edited