diff mbox series

[bug#63508,v2,4/4] gnu: eudev: Have udevadm look in /etc/udev/rules.d. (Closes: #63508)

Message ID e7aecfbe4642b43ea5837584b7666c87098ef15f.1684370595.git.felix.lechner@lease-up.com
State New
Headers show
Series [bug#63508,v2,1/4] gnu: eudev: Convert native-inputs to new style. | expand

Commit Message

Felix Lechner May 18, 2023, 12:52 a.m. UTC
Note for Liliana (and not part of the commit message): Hi, I hope you
are not offended by this patch. The one-line substitution here makes
the custom rule work, as well. The enviroment variable you proposed is
probably superior but the patch is relatively complex and the
resulting flexibility may not be needed. Also, I retitled the bug to
sidestep the controversy around the default for now. I was surprised
by your opposition and think that should be a separate
discussion. Thanks!

This substitution ensures that udevadm sees the rules that are actually in
effect for the declared operating system. It allows administrators to use the
udev-rules-service for network interfaces.

Some of Guix's customizations for udev rules appear to work as it is [1] but
that is not true for network interfaces (which invoke udevadm for naming
purposes). [2]

The author uses this snippet to select MAC-based names for all network
interfaces:

            (udev-rules-service 'net-name-mac
                                (udev-rule
                                 "79-net-name-mac.rules"
                                 "
SUBSYSTEM==\"net\", ACTION==\"add\", NAME=\"$env{ID_NET_NAME_MAC}\"
")))

Without this commit, udevadm will consult the rules that were present at build
time and were installed in the store).

[1] https://lists.gnu.org/archive/html/guix-devel/2023-05/msg00195.html
[2] https://lists.gnu.org/archive/html/guix-devel/2023-05/msg00192.html

* gnu/packages/linux.scm (eudev): Have udevadm look in
/etc/udev/rules.d. (Closes: #63508)
---
 gnu/packages/linux.scm | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Liliana Marie Prikler May 18, 2023, 4:19 a.m. UTC | #1
Am Mittwoch, dem 17.05.2023 um 17:52 -0700 schrieb Felix Lechner:
> Note for Liliana (and not part of the commit message):
Stuff that isn't part of the commit message ought to go below the
dashed (---) line so it's automatically ignored by git.

> Hi, I hope you are not offended by this patch. The one-line
> substitution here makes the custom rule work, as well. The enviroment
> variable you proposed is probably superior but the patch is
> relatively complex and the resulting flexibility may not be needed.
> Also, I retitled the bug to sidestep the controversy around the
> default for now. I was surprised by your opposition and think that
> should be a separate discussion. Thanks!
Note that our udev already uses this environment variable, I am only
changing how it is interpreted, i.e. allowing it to override built-in
rules just as is needed for your use case.  Now, you may object that
this doesn't mention /etc/udev/rules.d and thus could be problematic on
foreign distributions, but I argue that you probably shouldn't mess
with foreign udev anyway, and if you do that setting
EUDEV_RULES_DIRECTORY is appropriate.


Cheers
Felix Lechner May 28, 2023, 11:23 p.m. UTC | #2
Hi Liliana,

Thank you for your kind review! I will push a new version with
squashed commits you requested shortly.

While I am new to Guix, I am not sure that Gexp conversions fall under
"style changes" in my book. I believe they are considerably more
complex, and fraught with greater error.

On Wed, May 17, 2023 at 9:19 PM Liliana Marie Prikler
<liliana.prikler@gmail.com> wrote:
>
> you may object that
> this doesn't mention /etc/udev/rules.d and thus could be problematic on
> foreign distributions, but I argue that you probably shouldn't mess
> with foreign udev anyway, and if you do that setting
> EUDEV_RULES_DIRECTORY is appropriate.

The intent of my patch was not to mention /etc/udev/rules.d
explicitly, but rather to replace the store folder that holds the
upstream rules, which we are currently using, with the one Guix
constructs in order to use rules from other places. That just happens
to be /etc/udev/rules.d as well.

On that note, my patch is not suitable for upstream because it
hardcodes the location to the runtime path in Guix. Other
distributions may keep them in a different place. The current Autoconf
setup probably works well for them.

Either way, udevadm in Guix is currently broken. This patch fixes it
and should please be accepted. Thanks!

As noted elsewhere [1] I am separately working on an update to eudev
3.2.12 but that will require more testing locally before I can send it
in.

Kind regards
Felix

[1] https://lists.gnu.org/archive/html/guix-devel/2023-05/msg00217.html
Liliana Marie Prikler May 29, 2023, 8:29 a.m. UTC | #3
Am Sonntag, dem 28.05.2023 um 16:23 -0700 schrieb Felix Lechner:
> Hi Liliana,
> 
> Thank you for your kind review! I will push a new version with
> squashed commits you requested shortly.
You mean "submit", right?

> While I am new to Guix, I am not sure that Gexp conversions fall
> under "style changes" in my book. I believe they are considerably
> more complex, and fraught with greater error.
True, but there is room for error in dropping input labels as well.  In
fact, eudev's labels do cause a rebuild, but I decided to push v3 1/3
anyway to get CI ready.

> On Wed, May 17, 2023 at 9:19 PM Liliana Marie Prikler
> <liliana.prikler@gmail.com> wrote:
> > 
> > you may object that this doesn't mention /etc/udev/rules.d and thus
> > could be problematic on foreign distributions, but I argue that you
> > probably shouldn't mess with foreign udev anyway, and if you do
> > that setting EUDEV_RULES_DIRECTORY is appropriate.
> 
> The intent of my patch was not to mention /etc/udev/rules.d
> explicitly, but rather to replace the store folder that holds the
> upstream rules, which we are currently using, with the one Guix
> constructs in order to use rules from other places. That just happens
> to be /etc/udev/rules.d as well.
> 
> On that note, my patch is not suitable for upstream because it
> hardcodes the location to the runtime path in Guix. Other
> distributions may keep them in a different place. The current
> Autoconf setup probably works well for them.
The same reason why your patch wouldn't fly upstream is why it won't
fly in Guix.  We do have to consider foreign distributions as well.

> Either way, udevadm in Guix is currently broken. This patch fixes it
> and should please be accepted. Thanks!
There is more than one way to fix a bug and I argue that the one you
have chosen is not the right one.  Granted, same could be said for my
patch, but you have yet to file a formal complaint.  The closest I can
recall is "the resulting flexibility may not be needed", but here we
are discussing foreign distros storing udev rules in some other
location. 

Cheers
diff mbox series

Patch

diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm
index 7b989a466c..750016d572 100644
--- a/gnu/packages/linux.scm
+++ b/gnu/packages/linux.scm
@@ -4156,6 +4156,11 @@  (define-public eudev
      (list
       #:phases
       #~(modify-phases %standard-phases
+          (add-before 'bootstrap 'hardcode-runtime-rules-dir
+            (lambda _
+              (use-modules (ice-9 regex))
+              (substitute* "src/udev/Makefile.am"
+                (((regexp-quote "$(udevrulesdir)")) "/etc/udev/rules.d"))))
           (add-before 'bootstrap 'patch-file-names
             (lambda* (#:key inputs native-inputs #:allow-other-keys)
               (substitute* "man/make.sh"