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

repo: drop _AptCache and add migrate to install_stage_packages() #3030

Merged
merged 5 commits into from Apr 13, 2020

Conversation

cjp256
Copy link
Contributor

@cjp256 cjp256 commented Apr 10, 2020

This does remove the broken dependency handling... I need
a working test case to make sure that's solid.

  • Move repo.get_pkg_name_parts to test_base, even though it's
    in init.

  • Remove all instance methods from repo and Ubuntu, consolidating
    to install_stage_packages() classmethod.

  • Move non-class methods in Repo into Repo class so they can be
    easily inherited.

  • Remove custom _AptCache implementation.

    • Use a single instance of the system's apt cache, which should
      improve performance due to the penalty of recreating caches in
      numerous points. It also simplifies usage without the requirement
      of copying/sharing instances for what is (now) a system-wide
      resource.

    • Remove old scheme for gathering stage packages, and simply do a scan
      ourselves for the packages and their dependencies. Then fetch them
      using python-apt's fetch_binary().

  • Rework apt tests.

  • Rework of the ROS plugins to remove their current usage of Ubuntu
    and replace it with install_stage_packages().

Signed-off-by: Chris Patterson chris.patterson@canonical.com

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh tests/unit?

This does remove the broken dependency handling... I need
a working test case to make sure that's solid.

- Move repo.get_pkg_name_parts to test_base, even though it's
  in __init__.

- Remove all instance methods from repo and Ubuntu, consolidating
  to install_stage_packages() classmethod.

- Move non-class methods in Repo into Repo class so they can be
  easily inherited.

- Remove custom _AptCache implementation.

  - Use a single instance of the system's apt cache, which should
    improve performance due to the penalty of recreating caches in
    numerous points.  It also simplifies usage without the requirement
    of copying/sharing instances for what is (now) a system-wide
    resource.

  - Remove old scheme for gathering stage packages, and simply do a scan
    ourselves for the packages and their dependencies.  Then fetch them
    using python-apt's fetch_binary().

- Rework apt tests.

- Rework of the ROS plugins to remove their current usage of Ubuntu
  and replace it with install_stage_packages().

Signed-off-by: Chris Patterson <chris.patterson@canonical.com>
Copy link
Collaborator

@sergiusens sergiusens left a comment

Choose a reason for hiding this comment

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

Some initial comments

# TODO: rename with migration strategy.
repo_dir = path.join(self._project.parts_dir, part_name, "ubuntu")
stage_packages_repo = repo.Repo(repo_dir)
stage_packages_repo = repo.Repo
Copy link
Collaborator

Choose a reason for hiding this comment

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

repo_dir is used in PluginHandler, so you might need to adapt some tests or actual code.


@classmethod
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe

Suggested change
@classmethod
@staticmethod

and drop cls

elif os.path.exists(path):
_fix_filemode(path)

if path.endswith(".pc") and not os.path.islink(path):
fix_pkg_config(unpackdir, path)

def _fix_xml_tools(self, unpackdir: str) -> None:
@classmethod
Copy link
Collaborator

Choose a reason for hiding this comment

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

static

@@ -293,9 +267,10 @@ def _fix_xml_tools(self, unpackdir: str) -> None:
"prefix={}/usr".format(unpackdir),
)

def _fix_symlink(self, path: str, unpackdir: str, root: str) -> None:
@classmethod
Copy link
Collaborator

Choose a reason for hiding this comment

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

static

@@ -307,7 +282,8 @@ def _fix_symlink(self, path: str, unpackdir: str, root: str) -> None:
os.remove(path)
os.symlink(os.path.relpath(target, root), path)

def _fix_shebangs(self, unpackdir: str) -> None:
@classmethod
Copy link
Collaborator

Choose a reason for hiding this comment

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

static

@@ -517,10 +517,6 @@ def clean_pull(self):
if self.is_clean(steps.PULL):
return

# Remove stage-packages cache (where stage packages are fetched)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean that if you remove a stage-packages entry from a list that would remove the need to bring set packages in that those packages would still be staged?

Copy link
Contributor Author

@cjp256 cjp256 Apr 13, 2020

Choose a reason for hiding this comment

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

the cache directory is common for everything now (~/.cache/snapcraft/download). install_stage_packages() is the one that extracts the list of packages. So assuming the part was cleaned (that is, pluginhandler detects the change in stage-packages and rebuilds), that stage package is never extracted.

@cjp256 cjp256 changed the title [WIP] repo: drop _AptCache and add migrate to install_stage_packages() repo: drop _AptCache and add migrate to install_stage_packages() Apr 13, 2020
@cjp256 cjp256 marked this pull request as ready for review April 13, 2020 19:27
@@ -543,7 +534,7 @@ def prepare_build(self, force=False):
self.makedirs()
# Stage packages are fetched and unpacked in the pull step, but we'll
# unpack again here just in case the build step has been cleaned.
self._unpack_stage_packages()
self._install_stage_packages()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might not be needed anymore for v2... we need to cleanup snapcraft.internal.lifecycle._runner when we have time.

Copy link
Collaborator

@sergiusens sergiusens left a comment

Choose a reason for hiding this comment

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

Nice cleanup, this is now readable again

@sergiusens sergiusens merged commit 6646762 into canonical:master Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants