Merge lp:~bjornt/landscape-client/apt-facade-install-package into lp:~landscape/landscape-client/trunk

[Björn Tillenius]

Make it possible to install a package using AptFacade.

The basic functionality is there, but there are still a few features missing, like returning the output from Apt in perfom_changes().

I still think it makes sense to merge this branch, since I've already spent a lot of time on it, and nothing will be broken by this change.

[Alberto Donato (ack)]

Looks good, +1

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Nice, I just have a few questions. +1

[1]

+ version.package.candidate = version

Can you elaborate a bit on why this is needed and what does it do? A comment would be good.

And these two lines too:

+ fixer.clear(version.package._pkg)
+ fixer.protect(version.package._pkg)

[2]

+ versions_to_be_installed = set(
+ package.candidate for package in self._cache.get_changes())
+ dependencies = versions_to_be_installed.difference(
+ self._package_installs)

I'm not sure what the get_changes() API returns, does this handle packages to be upgraded and removed as well?

[3]

+ extra_items=None):

Maybe s/extra_items/control_fields/ would be more clear.

[4]

+ def commit():
+ self.committed = True

Instead of creating an attribute on the test case it'd be more isolated to append an item to a local list, like:

        committed = []
        def commit():
            committed.append(True)

review: Approve

[Björn Tillenius (bjornt)]

On Mon, Nov 07, 2011 at 03:43:43PM -0000, Free Ekanayaka wrote:
> Review: Approve
>
> Nice, I just have a few questions. +1
>
> [1]
>
> + version.package.candidate = version
>
> Can you elaborate a bit on why this is needed and what does it do? A
> comment would be good.

Sure, I added a comment.

package.candidate is the version of the package that Apt thinks we
should install, i.e. the latest version of the package. So if we want to
install another version of the package, we need to set the candidate
version manually.

> And these two lines too:
>
> + fixer.clear(version.package._pkg)
> + fixer.protect(version.package._pkg)

Added comment. Short story, this is what mark_install(auto_fix=True)
would do. It's to try to prevent the resolver from removing packages
that we asked to install to resolve dependency errors.

> [2]
>
> + versions_to_be_installed = set(
> + package.candidate for package in self._cache.get_changes())
> + dependencies = versions_to_be_installed.difference(
> + self._package_installs)
>
> I'm not sure what the get_changes() API returns, does this handle
> packages to be upgraded and removed as well?

Well, get_changes() does handle it, but I don't handle it yet here. I
plan to deal with the different dependency resolutions after I've
implemented mark_upgrade() and mark_remove().

> [3]
>
> + extra_items=None):
>
> Maybe s/extra_items/control_fields/ would be more clear.

Sure.

> [4]
>
> + def commit():
> + self.committed = True
>
> Instead of creating an attribute on the test case it'd be more
> isolated to append an item to a local list, like:
>
> committed = []
> def commit():
> committed.append(True)

Fixed.

--
Björn Tillenius | https://launchpad.net/~bjornt

[lp:~bjornt/landscape-client/apt-facade-install-package]

412. By Björn Tillenius on 2011-11-08

Add comment.

413. By Björn Tillenius on 2011-11-08

Add another comment.

414. By Björn Tillenius on 2011-11-08

Rename parameter.

415. By Björn Tillenius on 2011-11-08

Use a local variable.

416. By Björn Tillenius on 2011-11-08

Merge trunk.