Skip to content
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

Merged
4 changes: 4 additions & 0 deletions snapcraft/_baseplugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,10 @@ def __init__(self, name, options, project=None):
else:
self.builddir = self.build_basedir

# By default, snapcraft does an in-source build. Set this property to
# True if that's not desired.
self.out_of_source_build = False

# The API
def pull(self):
"""Pull the source code and/or internal prereqs to build the part."""
Expand Down
29 changes: 25 additions & 4 deletions snapcraft/file_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,18 +106,27 @@ def link_or_copy(source: str, destination: str,
"""

try:
_link(source, destination, follow_symlinks)
link(source, destination, follow_symlinks=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=follow_symlinks)


def _link(source: str, destination: str, follow_symlinks: bool=False) -> None:
def link(source: str, destination: str, *,
follow_symlinks: bool=False) -> None:
"""Hard-link source and destination files.

:param str source: The source to which destination will be linked.
:param str destination: The destination to be linked to source.
:param bool follow_symlinks: Whether or not symlinks should be followed.

:raises SnapcraftCopyFileNotFoundError: If source doesn't exist.
"""
# Note that follow_symlinks doesn't seem to work for os.link, so we'll
# implement this logic ourselves using realpath.
source_path = source
Expand All @@ -137,7 +146,19 @@ 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:
"""Copy source and destination files.

This function overwrites the destination if it already exists, and also
tries to copy ownership information.

:param str source: The source to be copied to destination.
:param str destination: Where to put the copy.
:param bool follow_symlinks: Whether or not symlinks should be followed.

:raises SnapcraftCopyFileNotFoundError: If source doesn't exist.
"""
# 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):
Expand Down
6 changes: 5 additions & 1 deletion snapcraft/internal/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,16 @@ class StepOutdatedError(SnapcraftError):
'`snapcraft clean {parts_names} -s {step.name}`.'
)

def __init__(self, *, step, part, dirty_report=None, dependents=None):
def __init__(self, *, step, part, dirty_report=None, outdated_report=None,
dependents=None):
messages = []

if dirty_report:
messages.append(dirty_report.get_report())

if outdated_report:
messages.append(outdated_report.get_report())

if dependents:
humanized_dependents = formatting_utils.humanize_list(
dependents, 'and')
Expand Down
104 changes: 78 additions & 26 deletions snapcraft/internal/lifecycle/_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -184,34 +184,56 @@ def run(self, step: steps.Step, part_names=None):
# already been built --elopio - 20170713
pluginhandler.check_for_collisions(self.config.all_parts)
for part in parts:
dirty_report = self._cache.get_dirty_report(
part, current_step)
if dirty_report:
self._handle_dirty(
part, current_step, dirty_report, cli_config)
elif self._cache.has_step_run(part, current_step):
# By default, if a step has already run, don't run it
# again. However, automatically clean and re-run the
# step if all the following conditions apply:
#
# 1. The step is the exact step that was requested
# (not an earlier one)
# 2. The part was explicitly specified
if (part_names and current_step == step and
part.name in part_names):
getattr(self, '_re{}'.format(
current_step.name))(part)
else:
notify_part_progress(
part,
'Skipping {}'.format(current_step.name),
'(already ran)')
else:
getattr(self, '_run_{}'.format(
current_step.name))(part)
self._handle_step(
part_names, part, step, current_step, cli_config)

self._create_meta(step, processed_part_names)

def _handle_step(self, requested_part_names: Sequence[str],
part: pluginhandler.PluginHandler,
requested_step: steps.Step,
current_step: steps.Step, cli_config) -> None:
# 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of multiple returns here, it seems this could be handled very well with elif and a final else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Alright, this step has already run. In that case, a few different
# things need to happen:

# 1. By default, if a step has already run, don't run it again.
# However, automatically clean and re-run the step if all the
# following conditions apply:
#
# 1.1. The step is the exact step that was requested (not an
# earlier one).
# 1.2. The part was explicitly specified.
if (requested_part_names and current_step == requested_step and
part.name in requested_part_names):
getattr(self, '_re{}'.format(current_step.name))(part)
return

# 2. If a step has already run, it might be dirty, in which case we
# need to clean and run it again.
dirty_report = self._cache.get_dirty_report(part, current_step)
if dirty_report:
self._handle_dirty(part, current_step, dirty_report, cli_config)
return

# 3. If a step has already run, it might be outdated, in which case we
# need to update it (without cleaning if possible).
outdated_report = self._cache.get_outdated_report(
part, current_step)
if outdated_report:
self._handle_outdated(
part, current_step, outdated_report, cli_config)
return

# 4. The step has already run, and is up-to-date, no need to run it
# again.
notify_part_progress(
part, 'Skipping {}'.format(current_step.name), '(already ran)')

def _run_pull(self, part):
self._run_step(step=steps.PULL, part=part, progress='Pulling')

Expand Down Expand Up @@ -244,7 +266,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_step(self, *, step: steps.Step,
part: pluginhandler.PluginHandler):
common.reset_env()
all_dependencies = self.parts_config.get_dependencies(part.name)

Expand Down Expand Up @@ -277,11 +300,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_step(step=step, part=part)

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._complete_step(part, step)

def _complete_step(self, part, step):
self._cache.clear_step(part, step)
self._cache.add_step_run(part, step)
self.steps_were_run = True

Expand Down Expand Up @@ -316,6 +346,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):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as handle_dirty, can this take a dirty_action as a parameter instead of passing down the entire object?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 handle_dirty_definitions and handle_dirty_[local]_source?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

  1. Not run (we call this "clean")
  2. 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:

  1. 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).
  2. 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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_step(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._complete_step(part, step)
else:
getattr(self, '_re{}'.format(step.name))(part, '({})'.format(
outdated_report.get_summary()))


def notify_part_progress(part, progress, hint='', debug=False):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is old, but what is the difference?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just keep it green 😄

Copy link
Contributor Author

@kyrofa kyrofa Jun 27, 2018

Choose a reason for hiding this comment

The 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:
Expand Down
26 changes: 25 additions & 1 deletion snapcraft/internal/lifecycle/_status_cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ 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

def should_step_run(self, part: pluginhandler.PluginHandler,
Expand All @@ -48,9 +49,12 @@ def should_step_run(self, part: pluginhandler.PluginHandler,
A given step should run if it:
1. Hasn't yet run
2. Is dirty
3. Either (1) or (2) apply to any earlier steps in its lifecycle
3. Is outdated
4. Either (1), (2), or (3) apply to any earlier steps in the part's
lifecycle
"""
if (not self.has_step_run(part, step) or
self.get_outdated_report(part, step) is not None or
self.get_dirty_report(part, step) is not None):
return True

Expand Down Expand Up @@ -82,6 +86,17 @@ def has_step_run(self, part: pluginhandler.PluginHandler,
self._ensure_steps_run(part)
return step in self._steps_run[part.name]

def get_outdated_report(self, part, step):
"""Obtain the outdated report for a given step of the given part.

:param pluginhandler.PluginHandler part: Part in question.
:param steps.Step step: Step in question.
:return: Outdated report (could be None)
:rtype: pluginhandler.OutdatedReport
"""
self._ensure_outdated_report(part, step)
return self._outdated_reports[part.name][step]

def get_dirty_report(self, part: pluginhandler.PluginHandler,
step: steps.Step) -> pluginhandler.DirtyReport:
"""Obtain the dirty report for a given step of the given part.
Expand All @@ -107,6 +122,9 @@ def clear_step(self, part: pluginhandler.PluginHandler,
_remove_key(self._steps_run[part.name], step)
if not self._steps_run[part.name]:
_del_key(self._steps_run, part.name)
_del_key(self._outdated_reports[part.name], step)
if not self._outdated_reports[part.name]:
_del_key(self._outdated_reports, part.name)
_del_key(self._dirty_reports[part.name], step)
if not self._dirty_reports[part.name]:
_del_key(self._dirty_reports, part.name)
Expand All @@ -115,6 +133,12 @@ def _ensure_steps_run(self, part: pluginhandler.PluginHandler) -> None:
if part.name not in self._steps_run:
self._steps_run[part.name] = _get_steps_run(part)

def _ensure_outdated_report(self, part: pluginhandler.PluginHandler,
step: steps.Step) -> None:
if step not in self._outdated_reports[part.name]:
self._outdated_reports[part.name][step] = part.get_outdated_report(
step)

def _ensure_dirty_report(self, part: pluginhandler.PluginHandler,
step: steps.Step) -> None:
# If we already have a dirty report, bail
Expand Down