On Mon, Oct 17, 2016 at 1:08 AM, Holger Levsen <hol...@layer-acht.org> wrote:
> package: piuparts
> submitter: Alexander Thomas <alexander.tho...@esaturnus.com>
> x-debbugs-cc: Alexander Thomas <alexander.tho...@esaturnus.com>
> Hi Alexander,
> thanks for your bug report, I turned it into one so we don't loose track
> of it!

All right, thanks!

> On Fri, Oct 14, 2016 at 03:53:49PM +0200, Alexander Thomas wrote:
>> In July 2014, a patch was included to use `cp -al` instead of `cp -ax`
>> when the -e option is used and the chroot is on the same filesystem as
>> where piuparts is run. I wonder why this was included without making
>> it optional, or maybe why it was included at all.
> a quick grep in todays piuparts for "cp -al" (and for "cp" too) reveal no 
> hits.
> can you confirm that piuparts 0.72 is affected?

Yes, the relevant code is around line 876 in piuparts.py:
  if os.stat(dirname).st_dev == os.stat(self.name).st_dev:
    cmd += ["-al"]
    logging.debug("Hard linking %s to %s" % (dirname, self.name))
    cmd += ["-ax"]
    logging.debug("Copying %s into %s" % (dirname, self.name))

>> Hard-linking the chroot makes little sense unless there is a guarantee
>> that none of the existing files will be modified. The only difference
>> with just running the test in the provided chroot itself, is that new
>> or deleted files will not be reflected in the original directory.
>> Modifications to existing files however will be reflected in the
>> original chroot, because of the hard links. Because the whole point of
>> piuparts testing is to detect unexpected changes, at some point a
>> naughty package will clobber existing files, and this change will
>> persist.
>> Maybe the submitter of the patch mistook `cp -al` as a copy-on-write,
>> or didn't mind that this would make the -e option of piuparts
>> destructive.
>> In our setup,
> could you maybe expand a little bit on this, I'm curious (and having
> users is motivating!

We install our software by bundling its components in Debian packages.
When we release a patch, we distribute an updated set of packages that
are served from a local repository within the system. This allows to
simply do a dist-upgrade on all of the VMs and servers to apply the
patch. Of course we want to make sure such upgrades don't do anything
unexpected, therefore packages must pass a piuparts test before they
are released.

>> we perform multiple separate runs of piuparts, and it is
>> essential to start from a pristine copy of the same chroot every time.
>> We use the -e option to avoid having to re-generate and customize the
>> chroot for every test, or unpack the same tarball over and over again.
>> It is essential that the original chroot is not modified. Relying on
>> hard links method breaks this workflow unless we wrap every test
>> inside yet another cp -ax, which again makes everything slower.
>> If anyone sees a valid use case for the hard linked chroot, an option
>> to enable it separately should be added. Otherwise this patch should
>> be reverted.
> reading #754878 it tells me this code should only be used with the
> --existing-chroot option.


> Right now I'm too tired to actually look at the code itself… Git patches
> are also much welcome too! ;-)

Because I don't know what was the use case for the submitter of that
hard-linking patch, it is safest to preserve it, but make it optional
with a new --hard-link switch that is only relevant together with
--existing-chroot. This is also very easy to implement, it only
requires one extra test on a boolean. I have attached a patch that
does exactly this.

Alexander Thomas
diff --git a/piuparts.1.txt b/piuparts.1.txt
index a857f4d..5a9c4d0 100644
--- a/piuparts.1.txt
+++ b/piuparts.1.txt
@@ -111,6 +111,11 @@ The tarball can be created with the '-s' option, or you can use one that *pbuild
   Takes a comma separated list of package names and can be given multiple
+  When the --existing-chroot option is used, and the source directory is on the
+  same filesystem, hard-link files instead of copying them. This is faster, but
+  any modifications to files will be reflected in the originals.
 *-i* 'filename', *--ignore*='filename'::
   Add a filename to the list of filenames to be ignored when comparing changes before and after installation. By default, piuparts ignores files that always change during a package installation and uninstallation, such as *dpkg* status files. The filename should be relative to the root of the chroot (e.g., _var/lib/dpkg/status_). This option can be used as many times as necessary.
diff --git a/piuparts.py b/piuparts.py
index f69c955..dbf2b18 100644
--- a/piuparts.py
+++ b/piuparts.py
@@ -178,6 +178,7 @@ class Settings:
         self.lvm_snapshot_size = "1G"
         self.adt_virt = None
         self.existing_chroot = None
+        self.hard_link = False
         self.schroot = None
         self.end_meta = None
         self.save_end_meta = None
@@ -873,7 +874,7 @@ class Chroot:
         """Create chroot from an existing one."""
         # if on same device, make hard link
         cmd = ["cp"]
-        if os.stat(dirname).st_dev == os.stat(self.name).st_dev:
+        if settings.hard_link and os.stat(dirname).st_dev == os.stat(self.name).st_dev:
             cmd += ["-al"]
             logging.debug("Hard linking %s to %s" % (dirname, self.name))
@@ -2768,6 +2769,11 @@ def parse_command_line():
                            "chroot, instead of building a new one with " +
+    parser.add_option("--hard-link", default=False,
+                      action='store_true',
+                      help="When using --existing-chroot, and the source dir is on the same"
+                           "filesystem, hard-link files instead of copying them.")
     parser.add_option("-i", "--ignore", action="append", metavar="FILENAME",
                       help="Add FILENAME to list of filenames to be " +
@@ -3017,6 +3023,7 @@ def parse_command_line():
     settings.lvm_volume = opts.lvm_volume
     settings.lvm_snapshot_size = opts.lvm_snapshot_size
     settings.existing_chroot = opts.existing_chroot
+    settings.hard_link = opts.hard_link
     settings.schroot = opts.schroot
     settings.end_meta = opts.end_meta
     settings.save_end_meta = opts.save_end_meta

Reply via email to