Bug#841149: devscripts: Improvements for debrepro script

[Antonio Terceiro]

On Wed, Oct 19, 2016 at 12:21:41AM +0200, Guillem Jover wrote:
> Hi!
> 
> On Mon, 2016-10-17 at 23:14:01 -0400, James McCoy wrote:
> > Thanks for the patches!  I think the first patch is straight forward
> > enough.  I'll let Antonio comment on whether he wants to apply the other
> > two.  I just had one comment, inline.
> 
> Perfect thanks!
> 
> > > @@ -83,13 +81,9 @@ create_build_script() {
> > >  echo 'cd ../disorderfs'
> > >fi
> > >  
> > > -  echo
> > > -  echo 'dpkg-source --before-build .'
> > > -  echo 'fakeroot debian/rules clean'
> > > -
> > >variation date
> > > -  vary 'fakeroot debian/rules binary' \
> > > -'faketime "+213days +7hours +13minutes" fakeroot debian/rules binary'
> > > +  vary 'dpkg-buildpackage -b' \
> > > +'dpkg-buildpackage -b -r"faketime +213days+7hours+13minutes 
> > > fakeroot"'
> > 
> > Shouldn't these use "-us -uc" too?  The intention of debrepro is just to
> > check for variance, not to produce something to upload, so I don't think
> > it should default to signing.
> 
> Ah, indeed sorry, I've gotten used to not pass those anymore as I've
> set them as default on my ~/.config/dpkg/buildpackage.conf file. :)
> 
> Attached rebased and fixed patches.

Looks good (and works) for me. Just merged and pushed your patches.

Thanks


signature.asc
Description: PGP signature

Bug#841149: devscripts: Improvements for debrepro script

[Guillem Jover]

Hi!

On Mon, 2016-10-17 at 23:14:01 -0400, James McCoy wrote:
> Thanks for the patches!  I think the first patch is straight forward
> enough.  I'll let Antonio comment on whether he wants to apply the other
> two.  I just had one comment, inline.

Perfect thanks!

> > @@ -83,13 +81,9 @@ create_build_script() {
> >  echo 'cd ../disorderfs'
> >fi
> >  
> > -  echo
> > -  echo 'dpkg-source --before-build .'
> > -  echo 'fakeroot debian/rules clean'
> > -
> >variation date
> > -  vary 'fakeroot debian/rules binary' \
> > -'faketime "+213days +7hours +13minutes" fakeroot debian/rules binary'
> > +  vary 'dpkg-buildpackage -b' \
> > +'dpkg-buildpackage -b -r"faketime +213days+7hours+13minutes fakeroot"'
> 
> Shouldn't these use "-us -uc" too?  The intention of debrepro is just to
> check for variance, not to produce something to upload, so I don't think
> it should default to signing.

Ah, indeed sorry, I've gotten used to not pass those anymore as I've
set them as default on my ~/.config/dpkg/buildpackage.conf file. :)

Attached rebased and fixed patches.

Thanks,
Guillem
From c9be0604cf10da3fbda6bec1b87f2905c80dcf7a Mon Sep 17 00:00:00 2001
From: Guillem Jover 
Date: Tue, 18 Oct 2016 03:54:21 +0200
Subject: [PATCH 1/2] debrepro: Use dpkg-buildpackage instead of ad-hoc code

Part of the reproducible machinery is handled already by
dpkg-buildpackage, so there's no need to duplicate it. We can also
pass faketime+fakeroot as a normal gain-root-command.
---
 scripts/debrepro.sh | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/scripts/debrepro.sh b/scripts/debrepro.sh
index 38dd14f..e1444fb 100755
--- a/scripts/debrepro.sh
+++ b/scripts/debrepro.sh
@@ -56,8 +56,6 @@ create_build_script() {
   echo "# package"
   echo
 
-  echo 'export SOURCE_DATE_EPOCH=$(date -d "$(dpkg-parsechangelog -SDate)" +%s)'
-
   variation PATH
   vary '' 'export PATH="$PATH":/i/capture/the/path'
 
@@ -83,13 +81,9 @@ create_build_script() {
 echo 'cd ../disorderfs'
   fi
 
-  echo
-  echo 'dpkg-source --before-build .'
-  echo 'fakeroot debian/rules clean'
-
   variation date
-  vary 'fakeroot debian/rules binary' \
-'faketime "+213days +7hours +13minutes" fakeroot debian/rules binary'
+  vary 'dpkg-buildpackage -b -us -uc' \
+'dpkg-buildpackage -b -us -uc -r"faketime +213days+7hours+13minutes fakeroot"'
 }
 
 
-- 
2.9.3

From 123cba54129187c45e3241c434f866921b19b10f Mon Sep 17 00:00:00 2001
From: Guillem Jover 
Date: Tue, 18 Oct 2016 04:29:43 +0200
Subject: [PATCH 2/2] debrepro: Compare .changes files and fallback to use
 debdiff

We should be checking all artifacts generated not just .deb files,
this includes .udebs and by-hand artifacts. This will also allow
adding support for source comparison.

If diffoscope is not present fallback to debdiff which is better than
a simple cmp(1).
---
 scripts/debrepro.sh | 35 +++
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/scripts/debrepro.sh b/scripts/debrepro.sh
index e1444fb..e7b19dd 100755
--- a/scripts/debrepro.sh
+++ b/scripts/debrepro.sh
@@ -3,6 +3,7 @@
 # debrepro: a reproducibility tester for Debian packages
 #
 # © 2016 Antonio Terceiro 
+# Copyright © 2016 Guillem Jover 
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -100,28 +101,22 @@ build() {
   mv "$tmpdir/build" "$tmpdir/$which_build"
 }
 
-binmatch() {
-  cmp --silent "$1" "$2"
-}
-
 compare() {
   rc=0
-  for first_deb in "$tmpdir"/first/*.deb; do
-deb="$(basename "$first_deb")"
-second_deb="$tmpdir"/second/"$deb"
-if binmatch "$first_deb" "$second_deb"; then
-  echo "✓ $deb: binaries match"
-else
-  echo ""
-  rc=1
-  if which diffoscope >/dev/null; then
-diffoscope "$first_deb" "$second_deb" || true
-  else
-echo "✗ $deb: binaries don't match"
-  fi
-fi
-  done
-  if [ "$rc" -ne 0 ]; then
+  first_changes=$(echo "$tmpdir"/first/*.changes)
+  changes="$(basename "$first_changes")"
+  second_changes="$tmpdir/second/$changes"
+
+  if which diffoscope >/dev/null; then
+diffoscope "$first_changes" "$second_changes" || rc=1
+  else
+debdiff -q -d --control --controlfiles ALL \
+  "$first_changes" "$second_changes" || rc=1
+  fi
+  if [ "$rc" -eq 0 ]; then
+echo "✓ $changes: artifacts match"
+  else
+echo "✗ $changes: artifacts do not match"
 echo "E: package is not reproducible."
 if ! which diffoscope >/dev/null; then
   echo "I: install diffoscope for a deeper comparison between binaries"
-- 
2.9.3

Bug#841149: devscripts: Improvements for debrepro script

[James McCoy]

Thanks for the patches!  I think the first patch is straight forward
enough.  I'll let Antonio comment on whether he wants to apply the other
two.  I just had one comment, inline.

> From 607b2b8f5c12377efe9b81e4bdf8105a3f2a6c0c Mon Sep 17 00:00:00 2001
> From: Guillem Jover 
> Date: Tue, 18 Oct 2016 03:54:21 +0200
> Subject: [PATCH 2/3] debrepro: Use dpkg-buildpackage instead of ad-hoc code
> 
> Part of the reproducible machinery is handled already by
> dpkg-buildpackage, so there's no need to duplicate it. We can also
> pass faketime+fakeroot as a normal gain-root-command.
> ---
>  scripts/debrepro.sh | 10 ++
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/scripts/debrepro.sh b/scripts/debrepro.sh
> index 38dd14f..0003d22 100755
> --- a/scripts/debrepro.sh
> +++ b/scripts/debrepro.sh
> @@ -56,8 +56,6 @@ create_build_script() {
>echo "# package"
>echo
>  
> -  echo 'export SOURCE_DATE_EPOCH=$(date -d "$(dpkg-parsechangelog -SDate)" 
> +%s)'
> -
>variation PATH
>vary '' 'export PATH="$PATH":/i/capture/the/path'
>  
> @@ -83,13 +81,9 @@ create_build_script() {
>  echo 'cd ../disorderfs'
>fi
>  
> -  echo
> -  echo 'dpkg-source --before-build .'
> -  echo 'fakeroot debian/rules clean'
> -
>variation date
> -  vary 'fakeroot debian/rules binary' \
> -'faketime "+213days +7hours +13minutes" fakeroot debian/rules binary'
> +  vary 'dpkg-buildpackage -b' \
> +'dpkg-buildpackage -b -r"faketime +213days+7hours+13minutes fakeroot"'

Shouldn't these use "-us -uc" too?  The intention of debrepro is just to
check for variance, not to produce something to upload, so I don't think
it should default to signing.

Cheers,
-- 
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7  2D23 DFE6 91AE 331B A3DB

Bug#841149: devscripts: Improvements for debrepro script

[Guillem Jover]

Package: devscripts
Version: 2.16.8
Severity: wishlist
Tags: patch

Hi!

This is a patch serie with some improvements to the debrepro script.
The main theme is that it switches to use dpkg-buildpackage as the
driver to reduce complexity, and compares .changes files instead of
individual .deb, and fallsback to debdiff which should be better
than cmp(1).

Thanks,
Guillem
From cdd89046859b7c3a36f37bac17179c13aba6eee6 Mon Sep 17 00:00:00 2001
From: Guillem Jover 
Date: Tue, 18 Oct 2016 03:39:15 +0200
Subject: [PATCH 1/3] debrepro: Simplify and make the vary() function more
 clear

Always use two arguments, so that it's obvious from the call sites that
the first round contains an empty variation, instead of shuffling
arguments around.
---
 scripts/debrepro.sh | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/scripts/debrepro.sh b/scripts/debrepro.sh
index 400913b..38dd14f 100755
--- a/scripts/debrepro.sh
+++ b/scripts/debrepro.sh
@@ -38,11 +38,7 @@ variation() {
 
 vary() {
   local first="$1"
-  local second="${2:-}"
-  if [ -z "$second" ]; then
-second="$first"
-first=''
-  fi
+  local second="$2"
   if [ "$which_build" = 'first' ]; then
 if [ -n "$first" ]; then
   echo "$first"
@@ -63,7 +59,7 @@ create_build_script() {
   echo 'export SOURCE_DATE_EPOCH=$(date -d "$(dpkg-parsechangelog -SDate)" +%s)'
 
   variation PATH
-  vary 'export PATH="$PATH":/i/capture/the/path'
+  vary '' 'export PATH="$PATH":/i/capture/the/path'
 
   variation USER
   vary 'export USER=user1' 'export USER=user2'
-- 
2.9.3

From 607b2b8f5c12377efe9b81e4bdf8105a3f2a6c0c Mon Sep 17 00:00:00 2001
From: Guillem Jover 
Date: Tue, 18 Oct 2016 03:54:21 +0200
Subject: [PATCH 2/3] debrepro: Use dpkg-buildpackage instead of ad-hoc code

Part of the reproducible machinery is handled already by
dpkg-buildpackage, so there's no need to duplicate it. We can also
pass faketime+fakeroot as a normal gain-root-command.
---
 scripts/debrepro.sh | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/scripts/debrepro.sh b/scripts/debrepro.sh
index 38dd14f..0003d22 100755
--- a/scripts/debrepro.sh
+++ b/scripts/debrepro.sh
@@ -56,8 +56,6 @@ create_build_script() {
   echo "# package"
   echo
 
-  echo 'export SOURCE_DATE_EPOCH=$(date -d "$(dpkg-parsechangelog -SDate)" +%s)'
-
   variation PATH
   vary '' 'export PATH="$PATH":/i/capture/the/path'
 
@@ -83,13 +81,9 @@ create_build_script() {
 echo 'cd ../disorderfs'
   fi
 
-  echo
-  echo 'dpkg-source --before-build .'
-  echo 'fakeroot debian/rules clean'
-
   variation date
-  vary 'fakeroot debian/rules binary' \
-'faketime "+213days +7hours +13minutes" fakeroot debian/rules binary'
+  vary 'dpkg-buildpackage -b' \
+'dpkg-buildpackage -b -r"faketime +213days+7hours+13minutes fakeroot"'
 }
 
 
-- 
2.9.3

From 9a167da06f0c2b5aba0ae8012894334aa6c88bf4 Mon Sep 17 00:00:00 2001
From: Guillem Jover 
Date: Tue, 18 Oct 2016 04:29:43 +0200
Subject: [PATCH 3/3] debrepro: Compare .changes files and fallback to use
 debdiff

We should be checking all artifacts generated not just .deb files,
this includes .udebs and by-hand artifacts. This will also allow
adding support for source comparison.

If diffoscope is not present fallback to debdiff which is better than
a simple cmp(1).
---
 scripts/debrepro.sh | 35 +++
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/scripts/debrepro.sh b/scripts/debrepro.sh
index 0003d22..50685f8 100755
--- a/scripts/debrepro.sh
+++ b/scripts/debrepro.sh
@@ -3,6 +3,7 @@
 # debrepro: a reproducibility tester for Debian packages
 #
 # © 2016 Antonio Terceiro 
+# Copyright © 2016 Guillem Jover 
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -100,28 +101,22 @@ build() {
   mv "$tmpdir/build" "$tmpdir/$which_build"
 }
 
-binmatch() {
-  cmp --silent "$1" "$2"
-}
-
 compare() {
   rc=0
-  for first_deb in "$tmpdir"/first/*.deb; do
-deb="$(basename "$first_deb")"
-second_deb="$tmpdir"/second/"$deb"
-if binmatch "$first_deb" "$second_deb"; then
-  echo "✓ $deb: binaries match"
-else
-  echo ""
-  rc=1
-  if which diffoscope >/dev/null; then
-diffoscope "$first_deb" "$second_deb" || true
-  else
-echo "✗ $deb: binaries don't match"
-  fi
-fi
-  done
-  if [ "$rc" -ne 0 ]; then
+  first_changes=$(echo "$tmpdir"/first/*.changes)
+  changes="$(basename "$first_changes")"
+  second_changes="$tmpdir/second/$changes"
+
+  if which diffoscope >/dev/null; then
+diffoscope "$first_changes" "$second_changes" || rc=1
+  else
+debdiff -q -d --control --controlfiles ALL \
+  "$first_changes" "$second_changes" || rc=1
+  fi
+  if [ "$rc" -eq 0 ]; then
+echo "✓ $changes: artifacts match"
+  else
+echo "✗