many: detect local source changes #2167
Conversation
kyrofa
force-pushed
the
feature/1583718/detect_source_changes
branch
from
6b262c9
to
0c85aba
Compare
|
Hey, what a PR, it is quite big and lots of tests seem to be failing, so I'll leave it to you to fix them before moving forward with an actual review. |
kyrofa
force-pushed
the
feature/1583718/detect_source_changes
branch
from
0c85aba
to
3abc332
Compare
Codecov Report
@@ Coverage Diff @@
## master #2167 +/- ##
==========================================
- Coverage 91.32% 91.28% -0.04%
==========================================
Files 201 202 +1
Lines 12617 12757 +140
Branches 1874 1900 +26
==========================================
+ Hits 11522 11645 +123
- Misses 741 752 +11
- Partials 354 360 +6
Continue to review full report at Codecov.
|
Yeah I had to move the in-test XDG cache around, so integration tests are taking a few iterations. I'll ping you. |
The snapcraft CLI doesn't notice changes to local sources once the parts have been pulled: one must clean and re-pull in order to take them into account. Start comparing local source timestamps to the pull state timestamp, updating the pull (and all subsequent) steps as necessary. LP: #1583718 Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
kyrofa
force-pushed
the
feature/1583718/detect_source_changes
branch
from
3abc332
to
77481b3
Compare
|
Alright @sergiusens, this one should be good, now. |
snapcraft/file_utils.py
Outdated
|
|
||
|
|
||
| def _link(source: str, destination: str, follow_symlinks: bool=False) -> None: | ||
| def link(source: str, destination: str, follow_symlinks: bool=False) -> None: |
There was a problem hiding this comment.
now that they are public it would be nice to force kwargs and add some docstrings if you don't mind
There was a problem hiding this comment.
This API needs to be compatible with copy2 and link_or_copy, so the only kwarg we can force is follow_symlinks, but I agree that it's an improvement.
There was a problem hiding this comment.
Fixed, thank you for catching the lack of docs!
snapcraft/file_utils.py
Outdated
| @@ -137,7 +137,7 @@ def _link(source: str, destination: str, follow_symlinks: bool=False) -> None: | |||
| raise SnapcraftCopyFileNotFoundError(source) | |||
|
|
|||
|
|
|||
| def _copy(source: str, destination: str, follow_symlinks: bool=False) -> None: | |||
| def copy(source: str, destination: str, follow_symlinks: bool=False) -> None: | |||
| @@ -277,11 +285,18 @@ def _run_step(self, *, step: steps.Step, part, progress, hint=''): | |||
|
|
|||
| part = _replace_in_part(part) | |||
|
|
|||
| def _run_step(self, *, step: steps.Step, part, progress, hint=''): | |||
| self._prepare_to_run(step=step, part=part) | |||
There was a problem hiding this comment.
maybe _prepare_to_run_step?
There was a problem hiding this comment.
Went with _prepare_step so we have _{prepare,run,complete}_step.
| notify_part_progress(part, progress, hint) | ||
| getattr(part, step.name)() | ||
|
|
||
| # We know we just ran this step, so rather than check, manually twiddle | ||
| # the cache | ||
| self._step_complete(part, step) | ||
|
|
||
| def _step_complete(self, part, step): |
There was a problem hiding this comment.
_complete_step for parity?
| raise RuntimeError("source must be checked before it's updated") | ||
| self._update() | ||
|
|
||
| def _check(self, target: str): |
There was a problem hiding this comment.
why not just do the abc. thing here, you can leave the docstring, it will still be valid code.
There was a problem hiding this comment.
Because then we'd need to add it to all sources, even those that don't actually support doing this. Since the vast majority of sources don't support it, it seemed to make sense to assume a lack of support in the API, and have only that source that does support it provide an implementation.
| else: | ||
| getattr(self, '_re{}'.format(step.name))(part, '({})'.format( | ||
| outdated_report.get_summary())) | ||
|
|
||
|
|
||
| def notify_part_progress(part, progress, hint='', debug=False): |
There was a problem hiding this comment.
this is old, but what is the difference?
There was a problem hiding this comment.
I'm afraid I'm not clear on what you're saying, here. Are you asking me to change something?
| @@ -316,6 +331,28 @@ def _handle_dirty(self, part, step, dirty_report, cli_config): | |||
| getattr(self, '_re{}'.format(step.name))(part, hint='({})'.format( | |||
| dirty_report.get_summary())) | |||
|
|
|||
| def _handle_outdated(self, part, step, outdated_report, cli_config): | |||
There was a problem hiding this comment.
same as handle_dirty, can this take a dirty_action as a parameter instead of passing down the entire object?
There was a problem hiding this comment.
With the introduction of the outdated concept, I think this is just another case of dirty. Can we add more precise qualifiers like handle_dirty_definitions and handle_dirty_[local]_source?
There was a problem hiding this comment.
Let me give you my thought process. A step (for a specific part) can be in one of two states:
- Not run (we call this "clean")
- Run, which is further broken down into sub-states:
2.1. Complete
2.2. Needs to be cleaned and run again (we call this "dirty", i.e. it needs to be cleaned)
2.3. Needs to be run again, but does not need to be cleaned (in this PR we call this "outdated", i.e. it needs to be updated)
Both 2.2 and 2.3 are caused by something, which needs to be recorded somewhere so we can tell the user what it was. We do that with the {Dirty,Outdated}Report classes. This allows us to isolate concerns, separating the "what makes this {dirty,outdated}" from "oh, this is {dirty,outdated}, I need to take appropriate action." If we combine these reports into one, we have a few issues:
- We would need to come up with a new name, since "dirty" no longer works (i.e. the solution is not to clean it anymore).
- Concerns leak. Now lifecycle needs to know "Oh, the project properties changed, okay, I know I need to clean" or "Oh, an earlier step happened, I know I need to update". This spread of knowledge seems error-prone.
(1) isn't a big deal, we can figure that one out. The word "outdated" could be used to refer to both situations, I suppose. (2) we can probably solve by keeping things similar to how they are today, but have a master class composed of both a dirty and outdated report, and take action appropriately. That at least gives us a single report to deal with in the PluginHandler's API and thus the lifecycle, but other than that doesn't buy us much. Do you like that idea better?
There was a problem hiding this comment.
I did not say combine them, I just meant that "dirty" and "outdated" are easier to confuse and that we should narrow the scope of what it means down.
Your thought process is from the point of view of the action wrt lifecycle and my mind is trending towards the state of things (why is it dirty/outdated) whilst the report comes from a component that does not determine the future actions.
These two terms lead to confusions, is this dirty because it is outdated (so far that has been our logic, right?). The what is outdated is what I am asking to more clearly state in the method calls, checks and class names.
But yeah, by no means, unless it gets converted into the state class merge these two.
There was a problem hiding this comment.
If you are really stuck with the names, I have added a couple more comments that would satisfy me. My intention was that from the names of variables and classes it was crystal clear what it meant without having to read the code, I can compromise with a bit more documentation under the internal class names and methods called.
There was a problem hiding this comment.
I agree that this should be cleaned up, but as we discussed on the call, let's see if we can get this PR in a decent state with docs and then take another pass (in another PR) at improving it further.
| part, | ||
| 'Skipping {}'.format(current_step.name), | ||
| '(already ran)') | ||
| outdated_report = self._cache.get_outdated_report( |
There was a problem hiding this comment.
I have complications accepting this procedural flow, just the level of nesting.
How is dirty_report so different from outdated_report? How can we have a dirty_report if we haven't has_step_run? Do you think this can be shuffled a bit?
There was a problem hiding this comment.
That's because it's the ugliest thing on Earth. I tried to avoid touching too much here, but let me see what I can do.
There was a problem hiding this comment.
There, that's quite a bit better. Thoughts?
| self.plugin.statedir, steps.PULL) | ||
|
|
||
| # Not all sources support checking for updates | ||
| with contextlib.suppress(NotImplementedError): |
There was a problem hiding this comment.
ok, I see the reason for raising now, still would be nice to have a different exception to avoid over catching (or check the payload) to not swallow exceptions like this.
There was a problem hiding this comment.
Fixed with a custom exception.
| if os.path.exists(self.plugin.build_basedir): | ||
| shutil.rmtree(self.plugin.build_basedir) | ||
|
|
||
| # FIXME: It's not necessary to ignore here anymore since it's now |
| @@ -25,13 +26,79 @@ | |||
|
|
|||
| class Local(Base): | |||
|
|
|||
| def __init__(self, *args, copy_function=file_utils.link_or_copy, **kwargs): | |||
There was a problem hiding this comment.
Do we have a scenario for copy_function to be different?
There was a problem hiding this comment.
Yeah, right here. Didn't want to duplicate functionality.
|
|
||
| def _update(self): | ||
| # First, copy the directories | ||
| for directory in self._updated_directories: |
There was a problem hiding this comment.
this feels a lot like the pluginhandlers "migration" code. Maybe we need some generalization there (in the future)
There was a problem hiding this comment.
There are definitely some similarities in that they both operate on directories and files, but the pluginhandler does some special stuff as well.
| @@ -116,6 +116,13 @@ def setUp(self): | |||
| patcher_dirs.start() | |||
| self.addCleanup(patcher_dirs.stop) | |||
|
|
|||
| self.useFixture(fixtures.EnvironmentVariable( | |||
| self.useFixture(fixtures.EnvironmentVariable( | ||
| 'XDG_DATA_HOME', os.path.join(self.path, 'data'))) | ||
| # Use a separate path for XDG dirs, or changes there may be detected as | ||
| # source changes. |
There was a problem hiding this comment.
kudos on the code comment, makes sense
There was a problem hiding this comment.
Most annoying change I had to make in the PR! Touched so many things. I should have extracted it, in retrospect.
…ect_source_changes
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
| # If this step hasn't yet run, all we need to do is run it | ||
| if not self._cache.has_step_run(part, current_step): | ||
| getattr(self, '_run_{}'.format(current_step.name))(part) | ||
| return |
There was a problem hiding this comment.
instead of multiple returns here, it seems this could be handled very well with elif and a final else.
There was a problem hiding this comment.
We're fetching reports before checking them, which is heavy to do upfront if we don't actually need to, so it doesn't fit particularly well within an if/elif unless we want to nest within elses.
snapcraft/internal/sources/errors.py
Outdated
|
|
||
| fmt = ( | ||
| 'Failed to update source: ' | ||
| "{source!s} sources don't support updating" |
There was a problem hiding this comment.
Can you add a . to end the sentence please?
| @@ -249,6 +250,32 @@ def is_clean(self, step): | |||
| except errors.NoLatestStepError: | |||
| return True | |||
|
|
|||
| def is_outdated(self, step): | |||
| """Return true if the given step needs to be updated (no cleaning).""" | |||
There was a problem hiding this comment.
Can you clarify this doc string (what is in between parens).
There was a problem hiding this comment.
Done, though mostly by directing the reader to get_outdated_report().
| return self.get_outdated_report(step) is not None | ||
|
|
||
| def get_outdated_report(self, step: steps.Step): | ||
| """Return an OutdatedReport class describing why step is outdated. |
There was a problem hiding this comment.
Can you add a follow up paragraph on what it means to be outdated? Simil for what it means to be dirty in its counterpart.
There was a problem hiding this comment.
Done, for both outdated and dirty.
|
|
||
|
|
||
| class OutdatedReport: | ||
| """The OutdatedReport class explains why a given step is outdated.""" |
There was a problem hiding this comment.
can you further expand in a paragraph what it means to be outdated (conditions that trigger it)? Simil for DirtyReport please.
There was a problem hiding this comment.
Done, for both outdated and dirty.
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
| @@ -34,8 +35,8 @@ def __init__(self, config: _config.Config) -> None: | |||
| """ | |||
| self.config = config | |||
| self._steps_run = dict() # type: Dict[str, Set[steps.Step]] | |||
| self._outdated_reports = collections.defaultdict(dict) # type: _Report | |||
| self._dirty_reports = collections.defaultdict(dict) # type: _Report | |||
| self._outdated_reports = collections.defaultdict(dict) # type: _OutdatedReport # noqa | |||
There was a problem hiding this comment.
why noqa or why noqa with no qualifier, and if it is to prevent a line break, I'd rather change the line limit to 99 😉
Use your parens until then 😆
|
atta boy 😄 |
./runtests.sh static?./runtests.sh unit?The snapcraft CLI doesn't notice changes to local sources once the parts have been pulled: one must clean and re-pull in order to take them into account. This PR resolves LP: #1583718 by comparing local source timestamps to the pull state timestamp, and updating the pull (and all subsequent) steps as necessary.