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

[Sean Whitton]

Dear Holger, Andreas,

It would be great if you were able to respond to my questions in the
message below, so I can prepare a new version of the patch.

Thanks.

Sean

On Fri, Jun 03, 2016 at 08:29:30AM +0900, Sean Whitton wrote:
> 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



-- 
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/20160723/223159d2/attachment.sig>