[Piuparts-devel] Bug#825487: Bug#825487: Patch

[Sean Whitton]

Hello,

Thanks for the feedback :)

On Thu, Jun 02, 2016 at 08:42:56AM +0000, Holger Levsen wrote:
> On Thu, Jun 02, 2016 at 04:50:31PM +0900, Sean Whitton wrote:
> > +            # we need to pass --allow-downgrades because installing a
> > +            # deb with the same version as the version currently
> > +            # installed (i.e. reinstalling it) counts as a downgrade
> 
> why do we need to do that? If I have foo installed and run "apt-get
> install foo", it succeeds and if I say "apt-get install foo bar" it will
> simply install bar.

It doesn't seem to work like that when foo and bar are debs rather than
archive package names.  I tested my patch by building a package already
in the archive with sbuild.  The piuparts test where it installs the
archive version and then installs the .deb you just built (presumably
the upgrade test) failed because it called it a downgrade and wouldn't
proceed without passing that option.  So far as I can tell it's a
behavioural difference between `apt-get install ./foo.deb` and `dpkg -i
./foo.deb`.

> > - if settings.list_installed_files: -
> > self.list_installed_files(pre_info, self.save_meta_data())
> 
> I'm not sure you can just remove this code here.

I thought that the code was saving metadata before running before each
command.  Since I'm running only one command instead of two, I thought I
could remove one invocation.  Am I wrong about the purpose of that code?

On Thu, Jun 02, 2016 at 11:02:59AM +0200, Andreas Beckmann wrote:
> > +            tmp_files = [os.path.join("./tmp", name) for name in
> tmp_files]
> 
> Can't we use just an absolute path? /tmp?

I think so.  I'll test it.

> > + apt_get_install = ["apt-get", "-y", "--allow-downgrades"]
> 
> NACK as said by Holger.
> Testing reinstalls this way does not seem to be a valid usecase.
> An option might enable this though.

As explained above it will fail without this.

> >              apt_get_install.extend(settings.distro_config.get_target_flags(
> > os.environ["PIUPARTS_DISTRIBUTION"]))
> > apt_get_install.append("install") @@ -1174,18 +1181,12 @@ class
> > Chroot: if settings.list_installed_files: pre_info =
> > self.save_meta_data()
> 
> > -            (ret, out) = self.run(["dpkg", "-i"] + tmp_files, ignore_errors=True)
> 
> We need to keep the dpkg code for older releases,
> so we need to have either an option that there is a new apt
> (or better better a test whether we have apt >= 1.2 (is that the correct
> version?))

Looks to be >=1.1 based on the changelog (see the entry for 1.1~exp1).

> > - if settings.list_installed_files: -
> > self.list_installed_files(pre_info, self.save_meta_data())
> 
> That breaks some options that are not frequently used, not sure how to
> solve this.

What do you have in mind?

> > - - self.run(apt_get_install) + self.run(apt_get_install +
> > tmp_files)
> >  
> >              if settings.list_installed_files:
> >                  self.list_installed_files(pre_info, self.save_meta_data())
> 
> 
> disregarding the list_installed_files bits (or maybe continue using dpkg
> for them in any case) it should roughly look like this:
> 
> if (apt_get_can_install_debs) {
>     apt-get -y install the.deb
> } else {
>     dpkg -i the.deb
>     apt-get -y install
> }

I agree.  If you could answer my other questions I can go ahead and
implement this.

On Thu, Jun 02, 2016 at 09:38:18AM +0000, Holger Levsen wrote:
> and then I wonder, shouldn't this use "apt" instead of "apt-get"? (or
> maybe better: use apt or apt-get or aptitude, depending on user choice,
> defaulting to apt?)

I was told that scripts are meant to call `apt-get` not `apt` but it's
possible I was misinformed.

-- 
Sean Whitton
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/piuparts-devel/attachments/20160603/bafa41c2/attachment.sig>