[bug#37346] Move diffoscope from package-management to it's own module.
diff mbox series

Message ID 87ftl6cqfc.fsf@yucca
State New
Headers show
Series
  • [bug#37346] Move diffoscope from package-management to it's own module.
Related show

Commit Message

Vagrant Cascadian Sept. 8, 2019, 11:35 p.m. UTC
I was having infinite recursion issues importing additional modules such
as android.scm, bootloaders.scm and statistics.scm into
package-management.scm with use-modules calls, along these lines:

error: googletest: unbound variable
hint: Did you forget a `use-modules' form?
...
error: curl: unbound variable
hint: Did you forget a `use-modules' form?

guix build: error: diffoscope: unknown package

Moving diffoscope to it's own package module seemed to at least work
around the issue. Diffoscope itself attempts to deal with an arbitrary
and growing number of file types, so pulls in quite a few other package
modules, so at least splitting it into a separate module might limit the
impacts on other modules.

Attached are two patches attempting the split; comments welcome!

The first patch is the move itself, and also moves trydiffoscope; the
second patch adds the package modules that previously would fail to
import properly before the move.

This allows diffoscope to run a few additional tests, getting the test
suite down to only about 68 skipped tests (and falling); it was around
140 skipped tests before I started this mad rush...


live well,
  vagrant

Comments

Danny Milosavljevic Sept. 10, 2019, 11:46 p.m. UTC | #1
Hi Vagrant,

On Sun, 08 Sep 2019 16:35:19 -0700
Vagrant Cascadian <vagrant@reproducible-builds.org> wrote:

> I was having infinite recursion issues importing additional modules such
> as android.scm, bootloaders.scm and statistics.scm into
> package-management.scm

Yeah, I hate import cycles.

I've pushed your patches to guix master.

Thanks!
Ludovic Courtès Sept. 11, 2019, 12:13 p.m. UTC | #2
Hello!

Vagrant Cascadian <vagrant@reproducible-builds.org> skribis:

> I was having infinite recursion issues importing additional modules such
> as android.scm, bootloaders.scm and statistics.scm into
> package-management.scm with use-modules calls, along these lines:
>
> error: googletest: unbound variable
> hint: Did you forget a `use-modules' form?

Circular module dependencies alone shouldn’t cause these problems.
Problems arise when those modules that depend on each other refer to
variables exported by one another *at the top level*.

That typically happens if, say, in android.scm you’d do:

  (package (inherit diffoscope) …)

> Moving diffoscope to it's own package module seemed to at least work
> around the issue. Diffoscope itself attempts to deal with an arbitrary
> and growing number of file types, so pulls in quite a few other package
> modules, so at least splitting it into a separate module might limit the
> impacts on other modules.
>
> Attached are two patches attempting the split; comments welcome!

The split is a good idea anyhow!

> From 352058f2ef002e77df6633c00ba009088bf5e8bd Mon Sep 17 00:00:00 2001
> From: Vagrant Cascadian <vagrant@reproducible-builds.org>
> Date: Sun, 8 Sep 2019 14:36:33 -0700
> Subject: [PATCH 1/2] gnu: Move diffoscope and trydiffoscope to new
>  diffoscope.scm.
>
> * gnu/packages/package-management (diffoscope): Remove variable.
>   (trydiffoscope): Remove variable.
>   (use-modules): Remove modules only needed by diffoscope.
>   Update copyright information.
> * gnu/packages/diffoscope.scm: New file.
>   (diffoscope): Add variable.
>   (trydiffoscope): Add variable.

LGTM.

> From c4ef8545514b4d594ab6fc083c954a22eace3786 Mon Sep 17 00:00:00 2001
> From: Vagrant Cascadian <vagrant@reproducible-builds.org>
> Date: Sun, 8 Sep 2019 15:35:33 -0700
> Subject: [PATCH 2/2] gnu: diffoscope: Add additional test dependencies.
>
> * gnu/packages/diffoscope (diffoscope)[native-inputs]: Add abootimg, dtc,
>   and r-minimal.
>   (use-module): Add android, bootloaders and statistics, respectively.

Note that we don’t usually mention ‘use-module’ changes in commit logs.

Anyway, LGTM, thanks!

Ludo’.
Vagrant Cascadian Sept. 18, 2019, 5:45 p.m. UTC | #3
On 2019-09-11, Danny Milosavljevic wrote:
> On Sun, 08 Sep 2019 16:35:19 -0700
> Vagrant Cascadian <vagrant@reproducible-builds.org> wrote:
>
>> I was having infinite recursion issues importing additional modules such
>> as android.scm, bootloaders.scm and statistics.scm into
>> package-management.scm
>
> Yeah, I hate import cycles.
>
> I've pushed your patches to guix master.

They don't seem to be in master as of commit:

  cf48ea9539020bd6300033a8910ca951225582e6

I don't see any reverts, so curious what happened?


live well,
  vagrant
Vagrant Cascadian Sept. 19, 2019, 12:30 a.m. UTC | #4
On 2019-09-18, Vagrant Cascadian wrote:
> On 2019-09-11, Danny Milosavljevic wrote:
>> On Sun, 08 Sep 2019 16:35:19 -0700
>> Vagrant Cascadian <vagrant@reproducible-builds.org> wrote:
>>
>>> I was having infinite recursion issues importing additional modules such
>>> as android.scm, bootloaders.scm and statistics.scm into
>>> package-management.scm
>>
>> Yeah, I hate import cycles.
>>
>> I've pushed your patches to guix master.
>
> They don't seem to be in master as of commit:
>
>   cf48ea9539020bd6300033a8910ca951225582e6
>
> I don't see any reverts, so curious what happened?

I went ahead and (re?)pushed it, dropping the use-modules bits based on
ludo's review.

live well,
  vagrant
Danny Milosavljevic Sept. 19, 2019, 2:04 p.m. UTC | #5
Hi Vagrant,

On Wed, 18 Sep 2019 17:30:35 -0700
Vagrant Cascadian <vagrant@reproducible-builds.org> wrote:

> I went ahead and (re?)pushed it, dropping the use-modules bits based on
> ludo's review.

Thanks.  Not sure what happened, but I've checked it now, seems to be OK.

Patch
diff mbox series

From c4ef8545514b4d594ab6fc083c954a22eace3786 Mon Sep 17 00:00:00 2001
From: Vagrant Cascadian <vagrant@reproducible-builds.org>
Date: Sun, 8 Sep 2019 15:35:33 -0700
Subject: [PATCH 2/2] gnu: diffoscope: Add additional test dependencies.

* gnu/packages/diffoscope (diffoscope)[native-inputs]: Add abootimg, dtc,
  and r-minimal.
  (use-module): Add android, bootloaders and statistics, respectively.
---
 gnu/packages/diffoscope.scm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gnu/packages/diffoscope.scm b/gnu/packages/diffoscope.scm
index 6eb5c1d9fe..828e06a818 100644
--- a/gnu/packages/diffoscope.scm
+++ b/gnu/packages/diffoscope.scm
@@ -25,8 +25,10 @@ 
   #:use-module (gnu packages)
   #:use-module (gnu packages acl)
   #:use-module (gnu packages admin)
+  #:use-module (gnu packages android)
   #:use-module (gnu packages backup)
   #:use-module (gnu packages base)
+  #:use-module (gnu packages bootloaders)
   #:use-module (gnu packages cdrom)
   #:use-module (gnu packages check)
   #:use-module (gnu packages compression)
@@ -50,6 +52,7 @@ 
   #:use-module (gnu packages python-xyz)
   #:use-module (gnu packages sqlite)
   #:use-module (gnu packages ssh)
+  #:use-module (gnu packages statistics)
   #:use-module (gnu packages textutils)
   #:use-module (gnu packages video)
   #:use-module (gnu packages vim)
@@ -137,6 +140,7 @@ 
       (native-inputs `(("python-pytest" ,python-pytest)
                        ("python-chardet" ,python-chardet)
                        ;; test suite skips tests when tool is missing
+                       ("abootimg" ,abootimg)
                        ("bdb" ,bdb)
                        ("binutils" ,binutils)
                        ("bzip2" ,bzip2)
@@ -144,6 +148,7 @@ 
                        ("colord" ,colord)
                        ("cpio" ,cpio)
                        ("docx2txt" ,docx2txt)
+                       ("dtc" ,dtc)
                        ("e2fsprogs" ,e2fsprogs)
                        ("ffmpeg" ,ffmpeg)
                        ("gettext" ,gettext-minimal)
@@ -163,6 +168,7 @@ 
                        ("openssh" ,openssh)
                        ("pgpdump" ,pgpdump)
                        ("poppler" ,poppler)
+                       ("r-minimal" ,r-minimal)
                        ("rpm" ,rpm)
                        ("sng" ,sng)
                        ("sqlite" ,sqlite)
-- 
2.20.1