-
Notifications
You must be signed in to change notification settings - Fork 441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
many: detect local source changes #2167
Changes from 2 commits
77481b3
d64fe62
f1779f5
46bfd96
f6946f6
5a43815
e95e7e1
3885d28
841378c
6cc1d79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,18 +106,18 @@ def link_or_copy(source: str, destination: str, | |
""" | ||
|
||
try: | ||
_link(source, destination, follow_symlinks) | ||
link(source, destination, follow_symlinks) | ||
except OSError as e: | ||
if e.errno == errno.EEXIST and not os.path.isdir(destination): | ||
# os.link will fail if the destination already exists, so let's | ||
# remove it and try again. | ||
os.remove(destination) | ||
link_or_copy(source, destination, follow_symlinks) | ||
else: | ||
_copy(source, destination, follow_symlinks) | ||
copy(source, destination, follow_symlinks) | ||
|
||
|
||
def _link(source: str, destination: str, follow_symlinks: bool=False) -> None: | ||
def link(source: str, destination: str, follow_symlinks: bool=False) -> None: | ||
# Note that follow_symlinks doesn't seem to work for os.link, so we'll | ||
# implement this logic ourselves using realpath. | ||
source_path = source | ||
|
@@ -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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dito There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
# If os.link raised an I/O error, it may have left a file behind. Skip on | ||
# OSError in case it doesn't exist or is a directory. | ||
with suppress(OSError): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -202,10 +202,17 @@ def run(self, step: steps.Step, part_names=None): | |
getattr(self, '_re{}'.format( | ||
current_step.name))(part) | ||
else: | ||
notify_part_progress( | ||
part, | ||
'Skipping {}'.format(current_step.name), | ||
'(already ran)') | ||
outdated_report = self._cache.get_outdated_report( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have complications accepting this procedural flow, just the level of nesting. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There, that's quite a bit better. Thoughts? |
||
part, current_step) | ||
if outdated_report: | ||
self._handle_outdated( | ||
part, current_step, outdated_report, | ||
cli_config) | ||
else: | ||
notify_part_progress( | ||
part, | ||
'Skipping {}'.format(current_step.name), | ||
'(already ran)') | ||
else: | ||
getattr(self, '_run_{}'.format( | ||
current_step.name))(part) | ||
|
@@ -244,7 +251,8 @@ def _reprime(self, part, hint=''): | |
self._rerun_step( | ||
step=steps.PRIME, part=part, progress='Re-priming', hint=hint) | ||
|
||
def _run_step(self, *, step: steps.Step, part, progress, hint=''): | ||
def _prepare_to_run(self, *, step: steps.Step, | ||
part: pluginhandler.PluginHandler): | ||
common.reset_env() | ||
all_dependencies = self.parts_config.get_dependencies(part.name) | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Went with |
||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, done. |
||
self._cache.clear_step(part, step) | ||
self._cache.add_step_run(part, step) | ||
self.steps_were_run = True | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as handle_dirty, can this take a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the introduction of the outdated concept, I think this is just another case of dirty. Can we add more precise qualifiers like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me give you my thought process. A step (for a specific part) can be in one of two states:
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
(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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
dirty_action = cli_config.get_outdated_step_action() | ||
if not step.clean_if_dirty: | ||
if dirty_action == config.OutdatedStepAction.ERROR: | ||
raise errors.StepOutdatedError( | ||
step=step, part=part.name, outdated_report=outdated_report) | ||
|
||
update_function = getattr(part, 'update_{}'.format(step.name), None) | ||
if update_function: | ||
self._prepare_to_run(step=step, part=part) | ||
notify_part_progress( | ||
part, 'Updating {} step for'.format(step.name), '({})'.format( | ||
outdated_report.get_summary())) | ||
update_function() | ||
|
||
# We know we just ran this step, so rather than check, manually | ||
# twiddle the cache | ||
self._step_complete(part, step) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is old, but what is the difference? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just keep it green 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm afraid I'm not clear on what you're saying, here. Are you asking me to change something? |
||
if debug: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,7 @@ | |
from ._runner import Runner | ||
from ._patchelf import PartPatcher | ||
from ._dirty_report import Dependency, DirtyReport # noqa | ||
from ._outdated_report import OutdatedReport | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you clarify this doc string (what is in between parens). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, though mostly by directing the reader to |
||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, for both outdated and dirty. |
||
|
||
Returns None if step is not outdated. | ||
""" | ||
|
||
try: | ||
return getattr(self, 'check_{}'.format(step.name))() | ||
except AttributeError: | ||
with contextlib.suppress(errors.StepHasNotRunError): | ||
timestamp = self.step_timestamp(step) | ||
|
||
for previous_step in reversed(step.previous_steps()): | ||
# Has a previous step run since this one ran? Then this | ||
# step needs to be updated. | ||
with contextlib.suppress(errors.StepHasNotRunError): | ||
if timestamp < self.step_timestamp(previous_step): | ||
return OutdatedReport( | ||
previous_step_modified=previous_step) | ||
return None | ||
|
||
def is_dirty(self, step): | ||
"""Return true if the given step needs to be cleaned and run again.""" | ||
|
||
|
@@ -301,11 +328,6 @@ def mark_done(self, step, state=None): | |
self.plugin.statedir, step), 'w') as f: | ||
f.write(yaml.dump(state)) | ||
|
||
# We know we've only just completed this step, so make sure any later | ||
# steps don't have a saved state. | ||
for step in step.next_steps(): | ||
self.mark_cleaned(step) | ||
|
||
def mark_cleaned(self, step): | ||
state_file = states.get_step_state_file(self.plugin.statedir, step) | ||
if os.path.exists(state_file): | ||
|
@@ -349,6 +371,21 @@ def pull(self, force=False): | |
self._runner.pull() | ||
self.mark_pull_done() | ||
|
||
def check_pull(self): | ||
# Check to see if pull needs to be updated | ||
state_file = states.get_step_state_file( | ||
self.plugin.statedir, steps.PULL) | ||
|
||
# Not all sources support checking for updates | ||
with contextlib.suppress(NotImplementedError): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure thing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed with a custom exception. |
||
if self.source_handler.check(state_file): | ||
return OutdatedReport(source_updated=True) | ||
return None | ||
|
||
def update_pull(self): | ||
self.source_handler.update() | ||
self.mark_pull_done() | ||
|
||
def _do_pull(self): | ||
if self.source_handler: | ||
self.source_handler.pull() | ||
|
@@ -406,28 +443,48 @@ def prepare_build(self, force=False): | |
def build(self, force=False): | ||
self.makedirs() | ||
|
||
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 done | ||
# in the Local source. However, it's left here so that it continues to | ||
# work on old snapcraft trees that still have src symlinks. | ||
def ignore(directory, files): | ||
if directory == self.plugin.sourcedir: | ||
snaps = glob(os.path.join(directory, '*.snap')) | ||
if snaps: | ||
snaps = [os.path.basename(s) for s in snaps] | ||
return common.SNAPCRAFT_FILES + snaps | ||
if not self.plugin.out_of_source_build: | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wow, this is so old 😅 |
||
# done in the Local source. However, it's left here so that it | ||
# continues to work on old snapcraft trees that still have src | ||
# symlinks. | ||
def ignore(directory, files): | ||
if directory == self.plugin.sourcedir: | ||
snaps = glob(os.path.join(directory, '*.snap')) | ||
if snaps: | ||
snaps = [os.path.basename(s) for s in snaps] | ||
return common.SNAPCRAFT_FILES + snaps | ||
else: | ||
return common.SNAPCRAFT_FILES | ||
else: | ||
return common.SNAPCRAFT_FILES | ||
else: | ||
return [] | ||
return [] | ||
|
||
# No hard-links being used here in case the build process modifies | ||
# these files. | ||
shutil.copytree(self.plugin.sourcedir, self.plugin.build_basedir, | ||
symlinks=True, ignore=ignore) | ||
|
||
self._do_build() | ||
|
||
def update_build(self): | ||
if not self.plugin.out_of_source_build: | ||
# Use the local source to update. It's important to use | ||
# file_utils.copy instead of link_or_copy, as the build process | ||
# may modify these files | ||
source = sources.Local( | ||
self.plugin.sourcedir, self.plugin.build_basedir, | ||
copy_function=file_utils.copy) | ||
if not source.check(states.get_step_state_file( | ||
self.plugin.statedir, steps.BUILD)): | ||
return | ||
source.update() | ||
|
||
# No hard-links being used here in case the build process modifies | ||
# these files. | ||
shutil.copytree(self.plugin.sourcedir, self.plugin.build_basedir, | ||
symlinks=True, ignore=ignore) | ||
self._do_build() | ||
|
||
def _do_build(self): | ||
self._runner.prepare() | ||
self._runner.build() | ||
self._runner.install() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thank you for catching the lack of docs!