[bug#33329] gnu: Deprecate linux-module shpchp and tell user to remove it.

Message ID 290b28ec-cd1e-e5e8-275e-133771c7d4f3@riseup.net
State Accepted
Headers show
Series [bug#33329] gnu: Deprecate linux-module shpchp and tell user to remove it. | expand

Checks

Context Check Description
cbaines/applying patch success Successfully applied
cbaines/applying patch success Successfully applied

Commit Message

swedebugia Nov. 10, 2018, 12:03 p.m. UTC
Hi

Updated patch attached with a fix of parens and removed my changes to 
linux-modules.scm.

I look forward to the review.

Cheers

On 2018-11-09 23:47, swedebugia wrote:
>
> Hi
>
> Ricardo, you mentioned that this needs to be fixed.
>
> I made a patch to fix this. I did not test the patch as I have no idea 
> how to do that.
>
> I have not run the indent script on it.
>
> Se attachment
>
>
> -------- Forwarded Message --------
> Subject: 	Re: GuixSD system reconfigure error
> Date: 	Thu, 30 Aug 2018 17:22:53 +0200
> From: 	Ricardo Wurmus <rekado@elephly.net>
> To: 	Hebi Li <lihebi.com@gmail.com>
> CC: 	help-guix@gnu.org
>
>
>
>
> Hi,
>
>>      https://github.com/lihebi/dothebi/blob/master/config.scm
>
> I see you use the “shpchp” kernel module. This no longer exists in the
> latest version of the kernel Linux. That’s what this really bad error
> message tries to tell you:
>
>> Backtrace:
>>             8 (primitive-load "/gnu/store/vz7ci9rh483f2zps2cl174rd91b?")
>> In ice-9/eval.scm:
>>      619:8  7 (_ #f)
>>     626:19  6 (_ #<directory (guile-user) 7cf140>)
>>      159:9  5 (_ _)
>> In gnu/build/linux-modules.scm:
>>     184:47  4 (recursive-module-dependencies _ #:lookup-module _)
>>      98:14  3 (module-dependencies _)
>>      85:18  2 (modinfo-section-contents _)
>> In ice-9/ports.scm:
>>     439:11  1 (call-with-input-file #f #<procedure get-bytevector-al?> ?)
>> In unknown file:
>>             0 (open-file #f "r" #:encoding #f #:guess-encoding #f)
>>
>> ERROR: In procedure open-file:
>> Wrong type (expecting string): #f
>> builder for `/gnu/store/0ahsvp7wx52zzh1rywbdbq78llcwb7id-linux-modules.drv' failed with exit code 1
>
> (I only know this because I had the same problem.) Obviously, this
> error message should be changed.
>
> As a workaround you could switch to the LTS kernel where that module
> still exists.
>
> --
> Ricardo
>
>

Comments

Ludovic Courtès Nov. 10, 2018, 10:31 p.m. UTC | #1
Hello,

swedebugia <swedebugia@riseup.net> skribis:

> From 4d70dda8c2f119fc6ff9d221eae6f060ff1fcd98 Mon Sep 17 00:00:00 2001
> From: swedebugia <swedebugia@riseup.net>
> Date: Fri, 9 Nov 2018 22:52:12 +0100
> Subject: [PATCH] [V2] gnu: Check for linux-module shpchp and tell user to
>  remove it.
>
>  * gnu/system/mapped-devices.scm (check-device-initrd-modules): New if
>  statement raising a condition if shpchp is found.

I don’t think we should hard-code things like this: they would
accumulate over the years and become unmanageable.

If anything, what should be improved IMO is the error message you get
when specifying a module that is unavailable.  That’s not easily done
though since that happens at build time.

Thoughts?

Ludo’.
Brett Gilio Nov. 11, 2018, 12:15 a.m. UTC | #2
Ludovic Courtès writes:

> If anything, what should be improved IMO is the error message you get
> when specifying a module that is unavailable.  That’s not easily done
> though since that happens at build time.

I think Ludo is correct in this, an error message seems to be the only
option we have to ensure that maintainability and reproducibility are
respected. To add onto that, I was thinking that maybe it could be part
of the configuration process that we could modify to ensure that all of
the specified modules that are needed are available else it throws an
error?

Brett Gilio
swedebugia Nov. 11, 2018, 7:27 a.m. UTC | #3
On 2018-11-11 01:15, Brett Gilio wrote:
> 
> Ludovic Courtès writes:
> 
>> If anything, what should be improved IMO is the error message you get
>> when specifying a module that is unavailable.  That’s not easily done
>> though since that happens at build time.
> 
> I think Ludo is correct in this, an error message seems to be the only
> option we have to ensure that maintainability and reproducibility are
> respected. To add onto that, I was thinking that maybe it could be part
> of the configuration process that we could modify to ensure that all of
> the specified modules that are needed are available else it throws an
> error?

Ok. Would my patch have worked anyway?

So where would that error message be produced?
In gnu/build/linux-modules? load-linux-modules?
Ludovic Courtès Nov. 11, 2018, 11:27 a.m. UTC | #4
Brett Gilio <brettg@posteo.net> skribis:

> Ludovic Courtès writes:
>
>> If anything, what should be improved IMO is the error message you get
>> when specifying a module that is unavailable.  That’s not easily done
>> though since that happens at build time.
>
> I think Ludo is correct in this, an error message seems to be the only
> option we have to ensure that maintainability and reproducibility are
> respected. To add onto that, I was thinking that maybe it could be part
> of the configuration process that we could modify to ensure that all of
> the specified modules that are needed are available else it throws an
> error?

That already happens, but the check cannot be 100% accurate because it
relies on information from the currently running kernel, which is why
there’s the ‘--skip-checks’ option.

Ludo’.
Ludovic Courtès Nov. 11, 2018, 11:32 a.m. UTC | #5
swedebugia <swedebugia@riseup.net> skribis:

> On 2018-11-11 01:15, Brett Gilio wrote:
>>
>> Ludovic Courtès writes:
>>
>>> If anything, what should be improved IMO is the error message you get
>>> when specifying a module that is unavailable.  That’s not easily done
>>> though since that happens at build time.
>>
>> I think Ludo is correct in this, an error message seems to be the only
>> option we have to ensure that maintainability and reproducibility are
>> respected. To add onto that, I was thinking that maybe it could be part
>> of the configuration process that we could modify to ensure that all of
>> the specified modules that are needed are available else it throws an
>> error?
>
> Ok. Would my patch have worked anyway?

I spotted a typo: (eqv? (missing 'shpchp)) is wrong because ‘eqv?’
should take two arguments and ‘missing’ is not a procedure.

Apart from this the patch could have worked I guess.

> So where would that error message be produced?
> In gnu/build/linux-modules? load-linux-modules?

An error message is produced while building the initrd; see commit
4db7a9dc663c5b26e45ec35538bf68ff87acdf7b.

For now, what about closing this issue and opening a new one when we
have an idea on how to improve on this?

Thanks,
Ludo’.
swedebugia Nov. 11, 2018, 6:42 p.m. UTC | #6
On 2018-11-11 12:32, Ludovic Courtès wrote:
>> swedebugia <swedebugia@riseup.net> skribis:

snip

>> Ok. Would my patch have worked anyway?
> 
> I spotted a typo: (eqv? (missing 'shpchp)) is wrong because ‘eqv?’
> should take two arguments and ‘missing’ is not a procedure.

Ok. Thanks for taking your time.

> 
> Apart from this the patch could have worked I guess.
> 
>> So where would that error message be produced?
>> In gnu/build/linux-modules? load-linux-modules?
> 
> An error message is produced while building the initrd; see commit
> 4db7a9dc663c5b26e45ec35538bf68ff87acdf7b.

Good, I had not seen that.

> 
> For now, what about closing this issue and opening a new one when we
> have an idea on how to improve on this?

Fine with me :)
Ludovic Courtès Nov. 12, 2018, 8:42 a.m. UTC | #7
swedebugia <swedebugia@riseup.net> skribis:

> On 2018-11-11 12:32, Ludovic Courtès wrote:

[...]

>>> So where would that error message be produced?
>>> In gnu/build/linux-modules? load-linux-modules?
>>
>> An error message is produced while building the initrd; see commit
>> 4db7a9dc663c5b26e45ec35538bf68ff87acdf7b.
>
> Good, I had not seen that.
>
>>
>> For now, what about closing this issue and opening a new one when we
>> have an idea on how to improve on this?
>
> Fine with me :)

Alright, done!

Ludo’.

Patch

From 4d70dda8c2f119fc6ff9d221eae6f060ff1fcd98 Mon Sep 17 00:00:00 2001
From: swedebugia <swedebugia@riseup.net>
Date: Fri, 9 Nov 2018 22:52:12 +0100
Subject: [PATCH] [V2] gnu: Check for linux-module shpchp and tell user to
 remove it.

 * gnu/system/mapped-devices.scm (check-device-initrd-modules): New if
 statement raising a condition if shpchp is found.
---
 gnu/packages/cran.go.3Q3wbP   |  0
 gnu/system/mapped-devices.scm | 28 +++++++++++++++++++---------
 2 files changed, 19 insertions(+), 9 deletions(-)
 create mode 100644 gnu/packages/cran.go.3Q3wbP

diff --git a/gnu/packages/cran.go.3Q3wbP b/gnu/packages/cran.go.3Q3wbP
new file mode 100644
index 000000000..e69de29bb
diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
index a87466646..483f952c6 100644
--- a/gnu/system/mapped-devices.scm
+++ b/gnu/system/mapped-devices.scm
@@ -2,6 +2,7 @@ 
 ;;; Copyright © 2014, 2015, 2016, 2017, 2018 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2016 Andreas Enge <andreas@enge.fr>
 ;;; Copyright © 2017, 2018 Mark H Weaver <mhw@netris.org>
+;;; Copyright © 2018 swedebugia <swedebugia@riseup.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -142,13 +143,22 @@  DEVICE must be a \"/dev\" file name."
         ;; "usb_storage"), not file names (e.g., "usb-storage.ko").  This is
         ;; OK because we have machinery that accepts both the hyphen and the
         ;; underscore version.
-        (raise (condition
-                (&message
-                 (message (format #f (G_ "you may need these modules \
+        (if (eqv? (missing 'shpchp))
+                 ;; Tell user to remove shpchp from config.scm
+                 ;; True
+                 (raise (condition
+                         (&message
+                          (message (format #f (G_ "shpchp is no longer \
+needed because it has been included in linux-libre. Please remove it from \
+your config.scm to continue"))))))
+                 ;; Else
+                 (raise (condition
+                         (&message
+                          (message (format #f (G_ "you may need these modules \
 in the initrd for ~a:~{ ~a~}")
-                                  device missing)))
-                (&fix-hint
-                 (hint (format #f (G_ "Try adding them to the
+                                           device missing)))
+                         (&fix-hint
+                          (hint (format #f (G_ "Try adding them to the
 @code{initrd-modules} field of your @code{operating-system} declaration, along
 these lines:
 
@@ -161,9 +171,9 @@  these lines:
 
 If you think this diagnostic is inaccurate, use the @option{--skip-checks}
 option of @command{guix system}.\n")
-                               missing)))
-                (&error-location
-                 (location (source-properties->location location)))))))))
+                                        missing)))
+                         (&error-location
+                          (location (source-properties->location location))))))))))
 
 
 ;;;
-- 
2.18.0