[Buildroot] [PATCH v3 1/3] autobuild-run: initial implementation of check_reproducibility()

[Buildroot] [PATCH v3 1/3] autobuild-run: initial implementation of check_reproducibility()

[Atharva Lele]

For reproducible builds, we want to find out if there are any differences
between two builds having the same configuration, and to find the reason behind
them. The diffoscope tool looks inside different types of files to see where the
differences lie.

check_reproducibility() runs diffoscope on two output directories which are
expected in output/images and output/images-1. Since it uses objdump, it needs
to be provided the cross-compile prefix which is derived from the TARGET_CROSS
variable.

Since diffoscope may not be installed, we fall back to cmp for byte-by-byte
comparison. We add diffoscope to list of optional programs to avoid repeated
checking of its presence.

Signed-off-by: Atharva Lele <itsatharva@gmail.com>

---
Changes v2 -> v3:
  - Use file size of reproducible_results to check reproducibility status, rather than
    exit status of cmp or diff (suggested by arnout)
  - Rename results file (diffoscope_output.txt -> reproducible_results) to avoid confusion
  - Change handling of diffoscope output text file using with open()
  - Changed commit message to have all necessary info (suggested by arnout)
  - Removed leftover code from when I was using an exception to handle
    diffoscope presense (thanks to arnout)

Changes v1 -> v2:
  - move diffoscope output to results dir (suggested by arnout)
  - fix make printvars call
  - Add diffoscope to DEFAULT_OPTIONAL_PROGS

Signed-off-by: Atharva Lele <itsatharva@gmail.com>
---
 scripts/autobuild-run | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/scripts/autobuild-run b/scripts/autobuild-run
index 190a254..ba5b337 100755
--- a/scripts/autobuild-run
+++ b/scripts/autobuild-run
@@ -204,7 +204,7 @@ def get_branch():
 
 class SystemInfo:
     DEFAULT_NEEDED_PROGS = ["make", "git", "gcc"]
-    DEFAULT_OPTIONAL_PROGS = ["bzr", "java", "javac", "jar"]
+    DEFAULT_OPTIONAL_PROGS = ["bzr", "diffoscope", "java", "javac", "jar"]
 
     def __init__(self):
         self.needed_progs = list(self.__class__.DEFAULT_NEEDED_PROGS)
@@ -394,6 +394,43 @@ def stop_on_build_hang(monitor_thread_hung_build_flag,
                 break
         monitor_thread_stop_flag.wait(30)
 
+def check_reproducibility(**kwargs):
+    """Check reproducibility of builds
+
+    Use diffoscope on the built images, if diffoscope is not
+    installed, fallback to cmp
+    """
+
+    log = kwargs['log']
+    idir = "instance-%d" % kwargs['instance']
+    outputdir = os.path.join(idir, "output")
+    srcdir = os.path.join(idir, "buildroot")
+    reproducible_results = os.path.join(outputdir, "results", "reproducible_results")
+    # Using only tar images for now
+    build_1_image = os.path.join(outputdir, "images-1", "rootfs.tar")
+    build_2_image = os.path.join(outputdir, "images", "rootfs.tar")
+
+    with open(reproducible_results, 'w') as diff:
+        if kwargs['sysinfo'].has("diffoscope"):
+            # Prefix to point diffoscope towards cross-tools
+            prefix = subprocess.check_output(["make", "O=%s" % outputdir, "-C", srcdir, "printvars", "VARS=TARGET_CROSS"])
+            # Remove TARGET_CROSS= and \n from the string
+            prefix = prefix[13:-1]
+            log_write(log, "INFO: running diffoscope on images")
+            subprocess.call(["diffoscope", build_1_image, build_2_image,
+                                "--tool-prefix-binutils", prefix], stdout=diff, stderr=log)
+        else:
+            log_write(log, "INFO: diffoscope not installed, falling back to cmp")
+            subprocess.call(["cmp", "-b", "build_1_image", "build_2_image"], stdout=diff, stderr=log)
+    
+    if os.stat(reproducible_results).st_size > 0:
+        log_write(log, "INFO: Build is non-reproducible.")
+        return -1
+    
+    # rootfs images match byte-for-byte -> reproducible image
+    log_write(log, "INFO: Build is reproducible!")
+    return 0
+
 def do_build(**kwargs):
     """Run the build itself"""
 
-- 
2.20.1

[Buildroot] [PATCH v3 2/3] autobuild-run: initial implementation of do_reproducible_build()

[Atharva Lele]

This new function will call do_build() twice to produce two builds
and then check their reproducibility.

Signed-off-by: Atharva Lele <itsatharva@gmail.com>
Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

---
Changes v2 -> v3:
  - make clean outputs to logfile, similar to other make calls (suggested by y_morin)
  - Output dir needs abspath (suggested by y_morin)
  - Directly use make clean as arguments rather than putting it into a variable
  - Added missing period at end of commit message (suggested by arnout)

Changes v1 -> v2:
  - make clean outputs to devnull (suggested by arnout)
  - remove unnecessary arguments to make clean (suggested by arnout)

Signed-off-by: Atharva Lele <itsatharva@gmail.com>
---
 scripts/autobuild-run | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/scripts/autobuild-run b/scripts/autobuild-run
index ba5b337..5ba6092 100755
--- a/scripts/autobuild-run
+++ b/scripts/autobuild-run
@@ -492,6 +492,43 @@ def do_build(**kwargs):
     log_write(log, "INFO: build successful")
     return 0
 
+def do_reproducible_build(**kwargs):
+    """Run the builds for reproducibility testing
+
+    Build twice with the same configuration. Calls do_build() to
+    perform the actual build.
+    """
+
+    idir = "instance-%d" % kwargs['instance']
+    outputdir = os.path.abspath(os.path.join(idir, "output"))
+    srcdir = os.path.join(idir, "buildroot")
+    log = kwargs['log']
+    
+    # Start the first build
+    log_write(log, "INFO: Reproducible Build Test, starting build 1")
+    ret = do_build(**kwargs)
+    if ret != 0:
+        log_write(log, "INFO: build 1 failed, skipping build 2")
+        return ret
+    
+    # First build has been built, move files and start build 2
+    os.rename(os.path.join(outputdir, "images"), os.path.join(outputdir, "images-1"))
+
+    # Clean up build 1
+    f = open(os.path.join(outputdir, "logfile"), "w+")
+    subprocess.call(["make", "O=%s" % outputdir, "-C", srcdir, "clean"], stdout=f, stderr=f)
+
+    # Start the second build
+    log_write(log, "INFO: Reproducible Build Test, starting build 2")
+    ret = do_build(**kwargs)
+    if ret != 0:
+        log_write(log, "INFO: build 2 failed")
+        return ret
+
+    # Assuming both have built successfully
+    ret = check_reproducibility(**kwargs)
+    return ret
+
 def send_results(result, **kwargs):
     """Prepare and store/send tarball with results
 
-- 
2.20.1

[Buildroot] [PATCH v3 3/3] autobuild-run: do reproducible builds tests if BR2_REPRODUCIBLE=y

[Atharva Lele]

Signed-off-by: Atharva Lele <itsatharva@gmail.com>
Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

---
Changes v2 -> v3:
  - use reproducible flag so that .config i snot kept open for the entire
    build duration. (suggested by arnout)
  - remove reduntant logging (suggested by arnout)

Changes v1 -> v2:
  - add trailing newline character to BR2_REPRODUCIBLE=y (suggested by arnout)

Signed-off-by: Atharva Lele <itsatharva@gmail.com>
---
 scripts/autobuild-run | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/scripts/autobuild-run b/scripts/autobuild-run
index 5ba6092..4fbd507 100755
--- a/scripts/autobuild-run
+++ b/scripts/autobuild-run
@@ -727,7 +727,14 @@ def run_instance(**kwargs):
             log_write(kwargs['log'], "WARN: failed to generate configuration")
             continue
 
-        ret = do_build(**kwargs)
+        # Check if the build test is supposed to be a reproducible test
+        outputdir = os.path.abspath(os.path.join(idir, "output"))
+        with open(os.path.join(outputdir, ".config"), "r") as fconf:
+            reproducible = "BR2_REPRODUCIBLE=y\n" in fconf.read()
+            if reproducible:
+                ret = do_reproducible_build(**kwargs)
+            else:
+                ret = do_build(**kwargs)
 
         send_results(ret, **kwargs)
 
-- 
2.20.1

[Buildroot] [PATCH v3 1/3] autobuild-run: initial implementation of check_reproducibility()

[Yann E. MORIN]

Atharva, All,

On 2019-06-11 18:04 +0530, Atharva Lele spake thusly:
> For reproducible builds, we want to find out if there are any differences
> between two builds having the same configuration, and to find the reason behind
> them. The diffoscope tool looks inside different types of files to see where the
> differences lie.
> 
> check_reproducibility() runs diffoscope on two output directories which are
> expected in output/images and output/images-1. Since it uses objdump, it needs
> to be provided the cross-compile prefix which is derived from the TARGET_CROSS
> variable.
> 
> Since diffoscope may not be installed, we fall back to cmp for byte-by-byte
> comparison. We add diffoscope to list of optional programs to avoid repeated
> checking of its presence.
> 
> Signed-off-by: Atharva Lele <itsatharva@gmail.com>

Except for two minor coding rules (see below):

Reviewed-by: Yann E. MORIN <yann.morin.1998@free.fr>

However, for the future: it would be wonderfull if we could entice the
diffoscope developers to make it possible to use it directly from
python: diffoscope *is* a python module, after all.

However, it is cutrrently not very amenable at being used as a python
object (yet):

diffoscope.main():
    https://salsa.debian.org/reproducible-builds/diffoscope/blob/master/diffoscope/main.py#L695

which is a glorified wrapper to diffoscope.run_diffoscope():
    https://salsa.debian.org/reproducible-builds/diffoscope/blob/master/diffoscope/main.py#L600

which is itself a further glofied wrapper to diffoscope.compare_root_paths()
and diffoscope.Difference()...

But that's for later.

> ---
> Changes v2 -> v3:
>   - Use file size of reproducible_results to check reproducibility status, rather than
>     exit status of cmp or diff (suggested by arnout)
>   - Rename results file (diffoscope_output.txt -> reproducible_results) to avoid confusion
>   - Change handling of diffoscope output text file using with open()
>   - Changed commit message to have all necessary info (suggested by arnout)
>   - Removed leftover code from when I was using an exception to handle
>     diffoscope presense (thanks to arnout)
> 
> Changes v1 -> v2:
>   - move diffoscope output to results dir (suggested by arnout)
>   - fix make printvars call
>   - Add diffoscope to DEFAULT_OPTIONAL_PROGS
> 
> Signed-off-by: Atharva Lele <itsatharva@gmail.com>
> ---
>  scripts/autobuild-run | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/autobuild-run b/scripts/autobuild-run
> index 190a254..ba5b337 100755
> --- a/scripts/autobuild-run
> +++ b/scripts/autobuild-run
> @@ -204,7 +204,7 @@ def get_branch():
>  
>  class SystemInfo:
>      DEFAULT_NEEDED_PROGS = ["make", "git", "gcc"]
> -    DEFAULT_OPTIONAL_PROGS = ["bzr", "java", "javac", "jar"]
> +    DEFAULT_OPTIONAL_PROGS = ["bzr", "diffoscope", "java", "javac", "jar"]
>  
>      def __init__(self):
>          self.needed_progs = list(self.__class__.DEFAULT_NEEDED_PROGS)
> @@ -394,6 +394,43 @@ def stop_on_build_hang(monitor_thread_hung_build_flag,
>                  break
>          monitor_thread_stop_flag.wait(30)
>  
> +def check_reproducibility(**kwargs):
> +    """Check reproducibility of builds
> +
> +    Use diffoscope on the built images, if diffoscope is not
> +    installed, fallback to cmp
> +    """
> +
> +    log = kwargs['log']
> +    idir = "instance-%d" % kwargs['instance']
> +    outputdir = os.path.join(idir, "output")
> +    srcdir = os.path.join(idir, "buildroot")
> +    reproducible_results = os.path.join(outputdir, "results", "reproducible_results")
> +    # Using only tar images for now
> +    build_1_image = os.path.join(outputdir, "images-1", "rootfs.tar")
> +    build_2_image = os.path.join(outputdir, "images", "rootfs.tar")
> +
> +    with open(reproducible_results, 'w') as diff:
> +        if kwargs['sysinfo'].has("diffoscope"):
> +            # Prefix to point diffoscope towards cross-tools
> +            prefix = subprocess.check_output(["make", "O=%s" % outputdir, "-C", srcdir, "printvars", "VARS=TARGET_CROSS"])
> +            # Remove TARGET_CROSS= and \n from the string
> +            prefix = prefix[13:-1]
> +            log_write(log, "INFO: running diffoscope on images")
> +            subprocess.call(["diffoscope", build_1_image, build_2_image,
> +                                "--tool-prefix-binutils", prefix], stdout=diff, stderr=log)
> +        else:
> +            log_write(log, "INFO: diffoscope not installed, falling back to cmp")
> +            subprocess.call(["cmp", "-b", "build_1_image", "build_2_image"], stdout=diff, stderr=log)
> +    

They are invisible, but this empty line has 4 spaces.

> +    if os.stat(reproducible_results).st_size > 0:
> +        log_write(log, "INFO: Build is non-reproducible.")
> +        return -1
> +    

Ditto.

Regards,
Yann E. MORIN.

> +    # rootfs images match byte-for-byte -> reproducible image
> +    log_write(log, "INFO: Build is reproducible!")
> +    return 0
> +
>  def do_build(**kwargs):
>      """Run the build itself"""
>  
> -- 
> 2.20.1
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v3 2/3] autobuild-run: initial implementation of do_reproducible_build()

[Yann E. MORIN]

Atharva, All,

On 2019-06-11 18:04 +0530, Atharva Lele spake thusly:
> This new function will call do_build() twice to produce two builds
> and then check their reproducibility.
> 
> Signed-off-by: Atharva Lele <itsatharva@gmail.com>
> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

Reviewed-by: Yann E. MORIN <yann.morin.1998@free.fr>

Yet, a few comments, see below...

> ---
> Changes v2 -> v3:
>   - make clean outputs to logfile, similar to other make calls (suggested by y_morin)
>   - Output dir needs abspath (suggested by y_morin)
>   - Directly use make clean as arguments rather than putting it into a variable
>   - Added missing period at end of commit message (suggested by arnout)
> 
> Changes v1 -> v2:
>   - make clean outputs to devnull (suggested by arnout)
>   - remove unnecessary arguments to make clean (suggested by arnout)
> 
> Signed-off-by: Atharva Lele <itsatharva@gmail.com>
> ---
>  scripts/autobuild-run | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/scripts/autobuild-run b/scripts/autobuild-run
> index ba5b337..5ba6092 100755
> --- a/scripts/autobuild-run
> +++ b/scripts/autobuild-run
> @@ -492,6 +492,43 @@ def do_build(**kwargs):
>      log_write(log, "INFO: build successful")
>      return 0
>  
> +def do_reproducible_build(**kwargs):
> +    """Run the builds for reproducibility testing
> +
> +    Build twice with the same configuration. Calls do_build() to
> +    perform the actual build.
> +    """
> +
> +    idir = "instance-%d" % kwargs['instance']
> +    outputdir = os.path.abspath(os.path.join(idir, "output"))
> +    srcdir = os.path.join(idir, "buildroot")
> +    log = kwargs['log']
> +    

Empty line made of spaces.

> +    # Start the first build
> +    log_write(log, "INFO: Reproducible Build Test, starting build 1")
> +    ret = do_build(**kwargs)
> +    if ret != 0:
> +        log_write(log, "INFO: build 1 failed, skipping build 2")
> +        return ret
> +    

Ditto.

> +    # First build has been built, move files and start build 2
> +    os.rename(os.path.join(outputdir, "images"), os.path.join(outputdir, "images-1"))
> +
> +    # Clean up build 1
> +    f = open(os.path.join(outputdir, "logfile"), "w+")
> +    subprocess.call(["make", "O=%s" % outputdir, "-C", srcdir, "clean"], stdout=f, stderr=f)

Since we do create a small Makefile wrapper in $(O), you could simplify
this a little with:

    subprocess.call(["make", "-C", outputdir, "clean"], stdout=f, stderr=f)

Ditto in the existing do_build() function. Can be improved upon when you
introduce the Builder class.

Regards,
Yann E. MORIN.

> +    # Start the second build
> +    log_write(log, "INFO: Reproducible Build Test, starting build 2")
> +    ret = do_build(**kwargs)
> +    if ret != 0:
> +        log_write(log, "INFO: build 2 failed")
> +        return ret
> +
> +    # Assuming both have built successfully
> +    ret = check_reproducibility(**kwargs)
> +    return ret
> +
>  def send_results(result, **kwargs):
>      """Prepare and store/send tarball with results
>  
> -- 
> 2.20.1
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v3 3/3] autobuild-run: do reproducible builds tests if BR2_REPRODUCIBLE=y

[Yann E. MORIN]

On 2019-06-11 18:04 +0530, Atharva Lele spake thusly:
> Signed-off-by: Atharva Lele <itsatharva@gmail.com>
> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> 
> ---
> Changes v2 -> v3:
>   - use reproducible flag so that .config i snot kept open for the entire
>     build duration. (suggested by arnout)

Sorry, but that is still the case, see below...

>   - remove reduntant logging (suggested by arnout)
> 
> Changes v1 -> v2:
>   - add trailing newline character to BR2_REPRODUCIBLE=y (suggested by arnout)
> 
> Signed-off-by: Atharva Lele <itsatharva@gmail.com>
> ---
>  scripts/autobuild-run | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/autobuild-run b/scripts/autobuild-run
> index 5ba6092..4fbd507 100755
> --- a/scripts/autobuild-run
> +++ b/scripts/autobuild-run
> @@ -727,7 +727,14 @@ def run_instance(**kwargs):
>              log_write(kwargs['log'], "WARN: failed to generate configuration")
>              continue
>  
> -        ret = do_build(**kwargs)
> +        # Check if the build test is supposed to be a reproducible test
> +        outputdir = os.path.abspath(os.path.join(idir, "output"))
> +        with open(os.path.join(outputdir, ".config"), "r") as fconf:
> +            reproducible = "BR2_REPRODUCIBLE=y\n" in fconf.read()
> +            if reproducible:
> +                ret = do_reproducible_build(**kwargs)
> +            else:
> +                ret = do_build(**kwargs)

This whole if reproducible block is still indented inside the with
block, so you'r still having .config opened during the whole build.

You need to do (basically):

    with open(os.path.join(outputdir, ".config"), "r") as fconf:
        reproducible = "BR2_REPRODUCIBLE=y\n" in fconf.read()
    build = do_reproducible_build if reproducible else do_build
    ret = build(**kwargs)

Regards,
Yann E. MORIN.

>  
>          send_results(ret, **kwargs)
>  
> -- 
> 2.20.1
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v3 2/3] autobuild-run: initial implementation of do_reproducible_build()

[Arnout Vandecappelle]

On 13/06/2019 22:13, Yann E. MORIN wrote:
> Atharva, All,
> 
> On 2019-06-11 18:04 +0530, Atharva Lele spake thusly:
[snip]
>> +    # Clean up build 1
>> +    f = open(os.path.join(outputdir, "logfile"), "w+")
>> +    subprocess.call(["make", "O=%s" % outputdir, "-C", srcdir, "clean"], stdout=f, stderr=f)
> 
> Since we do create a small Makefile wrapper in $(O), you could simplify
> this a little with:
> 
>     subprocess.call(["make", "-C", outputdir, "clean"], stdout=f, stderr=f)
> 
> Ditto in the existing do_build() function. Can be improved upon when you
> introduce the Builder class.

 This is bikeshedding, obviously, but I don't like that proposal. I think an
explicit O= -C is easier to read and understand.

 Shorter is not better :-)

 Regards,
 Arnout

[Buildroot] [PATCH v3 3/3] autobuild-run: do reproducible builds tests if BR2_REPRODUCIBLE=y

[Atharva Lele]

> You need to do (basically):

>    with open(os.path.join(outputdir, ".config"), "r") as fconf:
>        reproducible = "BR2_REPRODUCIBLE=y\n" in fconf.read()
>    build = do_reproducible_build if reproducible else do_build
>    ret = build(**kwargs)

It would still work if I just unindent the if-else block too right? This is
what I actually intended to do
It's working for me in testing..
Like this:

    with open(os.path.join(outputdir, ".config"), "r") as fconf:
        reproducible = "BR2_REPRODUCIBLE=y\n" in fconf.read()
    if reproducible:
        ret = do_reproducible_build(**kwargs)
    else:
        ret = do_build(**kwargs)

Regards,
Atharva Lele


On Fri, Jun 14, 2019 at 1:53 AM Yann E. MORIN <yann.morin.1998@free.fr>
wrote:

> On 2019-06-11 18:04 +0530, Atharva Lele spake thusly:
> > Signed-off-by: Atharva Lele <itsatharva@gmail.com>
> > Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> >
> > ---
> > Changes v2 -> v3:
> >   - use reproducible flag so that .config i snot kept open for the entire
> >     build duration. (suggested by arnout)
>
> Sorry, but that is still the case, see below...
>
> >   - remove reduntant logging (suggested by arnout)
> >
> > Changes v1 -> v2:
> >   - add trailing newline character to BR2_REPRODUCIBLE=y (suggested by
> arnout)
> >
> > Signed-off-by: Atharva Lele <itsatharva@gmail.com>
> > ---
> >  scripts/autobuild-run | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/autobuild-run b/scripts/autobuild-run
> > index 5ba6092..4fbd507 100755
> > --- a/scripts/autobuild-run
> > +++ b/scripts/autobuild-run
> > @@ -727,7 +727,14 @@ def run_instance(**kwargs):
> >              log_write(kwargs['log'], "WARN: failed to generate
> configuration")
> >              continue
> >
> > -        ret = do_build(**kwargs)
> > +        # Check if the build test is supposed to be a reproducible test
> > +        outputdir = os.path.abspath(os.path.join(idir, "output"))
> > +        with open(os.path.join(outputdir, ".config"), "r") as fconf:
> > +            reproducible = "BR2_REPRODUCIBLE=y\n" in fconf.read()
> > +            if reproducible:
> > +                ret = do_reproducible_build(**kwargs)
> > +            else:
> > +                ret = do_build(**kwargs)
>
> This whole if reproducible block is still indented inside the with
> block, so you'r still having .config opened during the whole build.
>
> You need to do (basically):
>
>     with open(os.path.join(outputdir, ".config"), "r") as fconf:
>         reproducible = "BR2_REPRODUCIBLE=y\n" in fconf.read()
>     build = do_reproducible_build if reproducible else do_build
>     ret = build(**kwargs)
>
> Regards,
> Yann E. MORIN.
>
> >
> >          send_results(ret, **kwargs)
> >
> > --
> > 2.20.1
> >
>
> --
>
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics'
> conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___
>      |
> | +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is
> no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v
>  conspiracy.  |
>
> '------------------------------^-------^------------------^--------------------'
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20190614/6362ef8b/attachment.html>

[Buildroot] [PATCH v3 2/3] autobuild-run: initial implementation of do_reproducible_build()

[Atharva Lele]

> I think an explicit O= -C is easier to read and understand.

I do agree with this.

Regards,
Atharva Lele


On Fri, Jun 14, 2019 at 1:28 PM Arnout Vandecappelle <arnout@mind.be> wrote:

>
>
> On 13/06/2019 22:13, Yann E. MORIN wrote:
> > Atharva, All,
> >
> > On 2019-06-11 18:04 +0530, Atharva Lele spake thusly:
> [snip]
> >> +    # Clean up build 1
> >> +    f = open(os.path.join(outputdir, "logfile"), "w+")
> >> +    subprocess.call(["make", "O=%s" % outputdir, "-C", srcdir,
> "clean"], stdout=f, stderr=f)
> >
> > Since we do create a small Makefile wrapper in $(O), you could simplify
> > this a little with:
> >
> >     subprocess.call(["make", "-C", outputdir, "clean"], stdout=f,
> stderr=f)
> >
> > Ditto in the existing do_build() function. Can be improved upon when you
> > introduce the Builder class.
>
>  This is bikeshedding, obviously, but I don't like that proposal. I think
> an
> explicit O= -C is easier to read and understand.
>
>  Shorter is not better :-)
>
>  Regards,
>  Arnout
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20190614/259a3597/attachment.html>

[Buildroot] [PATCH v3 3/3] autobuild-run: do reproducible builds tests if BR2_REPRODUCIBLE=y

[Arnout Vandecappelle]

On 14/06/2019 10:46, Atharva Lele wrote:
>> You need to do (basically):
> 
>>? ? with open(os.path.join(outputdir, ".config"), "r") as fconf:
>>? ? ? ? reproducible = "BR2_REPRODUCIBLE=y\n" in fconf.read()
>>? ? build = do_reproducible_build if reproducible else do_build
>>? ? ret = build(**kwargs)
> 
> It would still work if I just unindent the if-else block too right? This is what
> I actually intended to do
> It's working for me in testing..
> Like this:
> 
> ? ? with open(os.path.join(outputdir, ".config"), "r") as fconf:
> ? ? ? ? reproducible = "BR2_REPRODUCIBLE=y\n" in fconf.read()
> ? ? if reproducible:
> ? ? ? ? ret = do_reproducible_build(**kwargs)
> ? ? else:
> ? ? ? ? ret = do_build(**kwargs)

 Yes, and I prefer that version. Yann considers the '... if ... else ...'
construct more pythonese, but I kind of like explicit conditions :-)

 Regards,
 Arnout


[snip]

[Buildroot] [PATCH v3 3/3] autobuild-run: do reproducible builds tests if BR2_REPRODUCIBLE=y

[Atharva Lele]

> Yann considers the '... if ... else ...' construct more pythonese,
> but I kind of like explicit conditions :-)

Haha I do see his point of view. For me, I've only worked with C for the
last 2 years at university and my last internship so it's a bit hard to
'talk' in
pythonese.

Regards,
Atharva Lele


On Fri, Jun 14, 2019 at 2:17 PM Arnout Vandecappelle <arnout@mind.be> wrote:

>
>
> On 14/06/2019 10:46, Atharva Lele wrote:
> >> You need to do (basically):
> >
> >>    with open(os.path.join(outputdir, ".config"), "r") as fconf:
> >>        reproducible = "BR2_REPRODUCIBLE=y\n" in fconf.read()
> >>    build = do_reproducible_build if reproducible else do_build
> >>    ret = build(**kwargs)
> >
> > It would still work if I just unindent the if-else block too right? This
> is what
> > I actually intended to do
> > It's working for me in testing..
> > Like this:
> >
> >     with open(os.path.join(outputdir, ".config"), "r") as fconf:
> >         reproducible = "BR2_REPRODUCIBLE=y\n" in fconf.read()
> >     if reproducible:
> >         ret = do_reproducible_build(**kwargs)
> >     else:
> >         ret = do_build(**kwargs)
>
>  Yes, and I prefer that version. Yann considers the '... if ... else ...'
> construct more pythonese, but I kind of like explicit conditions :-)
>
>  Regards,
>  Arnout
>
>
> [snip]
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20190614/e12fa2c2/attachment.html>

[Buildroot] [PATCH v3 3/3] autobuild-run: do reproducible builds tests if BR2_REPRODUCIBLE=y

[Yann E. MORIN]

Arnout, All,

On 2019-06-14 10:47 +0200, Arnout Vandecappelle spake thusly:
> On 14/06/2019 10:46, Atharva Lele wrote:
> >> You need to do (basically):
> > 
> >>? ? with open(os.path.join(outputdir, ".config"), "r") as fconf:
> >>? ? ? ? reproducible = "BR2_REPRODUCIBLE=y\n" in fconf.read()
> >>? ? build = do_reproducible_build if reproducible else do_build
> >>? ? ret = build(**kwargs)
> > 
> > It would still work if I just unindent the if-else block too right? This is what
> > I actually intended to do
> > It's working for me in testing..
> > Like this:
> > 
> > ? ? with open(os.path.join(outputdir, ".config"), "r") as fconf:
> > ? ? ? ? reproducible = "BR2_REPRODUCIBLE=y\n" in fconf.read()
> > ? ? if reproducible:
> > ? ? ? ? ret = do_reproducible_build(**kwargs)
> > ? ? else:
> > ? ? ? ? ret = do_build(**kwargs)
> 
>  Yes, and I prefer that version. Yann considers the '... if ... else ...'
> construct more pythonese, but I kind of like explicit conditions :-)

Well, if we use a language, we should write with the idioms of that
language. Python is not C, so we should not write Python as we would C.

That notwithstanding, I'm no Python expert either, so I'm also OK with
the more "traditional and conventional" ways. ;-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'