etc/pam.d/Makefile.am: Fix typo

alejandro-colomar commented 6 months ago

Fixes: 341d80c2c751 ("Makefile: move chpasswd and newusers to pamd target") Link: https://github.com/shadow-maint/shadow/pull/928#discussion_r1487687347 Link: https://github.com/shadow-maint/shadow/issues/926#issuecomment-1941324761 Reported-by: @DimStar77 Reported-by: @jubalh Cc: @loqs Cc: @dvzrv Cc: @ikerexxe

To be cherry-picked to 4.14.x.

alejandro-colomar commented 6 months ago

@DimStar77

Do you mean this?

diff --git a/etc/pam.d/Makefile.am b/etc/pam.d/Makefile.am
index fe676b44..6a76551f 100644
--- a/etc/pam.d/Makefile.am
+++ b/etc/pam.d/Makefile.am
@@ -2,6 +2,7 @@
 # and also cooperate to make a distribution for `make dist'

 pamd_files = \
+       chpasswd \
        chfn \
        chgpasswd \
        chsh \
@@ -12,7 +13,6 @@ pamd_files = \

 pamd_acct_tools_files = \
        chage \
-       chpasswd \
        groupadd \
        groupdel \
        groupmod \

or this?

diff --git a/etc/pam.d/Makefile.am b/etc/pam.d/Makefile.am
index fe676b44..b8e4321f 100644
--- a/etc/pam.d/Makefile.am
+++ b/etc/pam.d/Makefile.am
@@ -2,8 +2,8 @@
 # and also cooperate to make a distribution for `make dist'

 pamd_files = \
+       chpasswd \
        chfn \
-       chgpasswd \
        chsh \
        groupmems \
        login \
@@ -12,7 +12,7 @@ pamd_files = \

 pamd_acct_tools_files = \
        chage \
-       chpasswd \
+       chgpasswd \
        groupadd \
        groupdel \
        groupmod \

Or none?

DimStar77 commented 6 months ago

@DimStar77

2nd variant is more in line with the commit message of the original PR;

alejandro-colomar commented 6 months ago

It depends - the original commit message mentioned wanting to move chpasswd up - now it's chgpasswd..

Yeah, that's why I want someone with the original problem to test this thing. :) The problem was reproducible on Arch Linux, so if any of the packagers from Arch can check this, it would help. Did Suse or Gentoo have the original problem too maybe?

alejandro-colomar commented 6 months ago

@DimStar77

2nd variant is more in line with the commit message of the original PR;

Yep. Thanks!

DimStar77 commented 6 months ago

It depends - the original commit message mentioned wanting to move chpasswd up - now it's chgpasswd..

Yeah, that's why I want someone with the original problem to test this thing. :) The problem was reproducible on Arch Linux, so if any of the packagers from Arch can check this, it would help. Did Suse or Gentoo have the original problem too maybe?

At least openSUSE/SUSE did not - as we build shadow with --enable-account-tools-setuid

alejandro-colomar commented 6 months ago

v2 changes:

Respect the original commit message, so the typo seems somewhere else. Document it in the commit message. [@DimStar77 ]
Co-developed-by: @DimStar77

$ git range-diff shadow/master gh/tfix tfix 
1:  52af3050 < -:  -------- etc/pam.d/Makefile.am: Fix typo
-:  -------- > 1:  3ad184cc etc/pam.d/Makefile.am: Fix typo

alejandro-colomar commented 6 months ago

I've marked 4.14.4 as broken (https://github.com/shadow-maint/shadow/releases/tag/4.14.4), and set 4.14.3 as the latest release.

alejandro-colomar commented 6 months ago

I'll take your approval as a Signed-off-by ;)

alejandro-colomar commented 6 months ago

v2b changes:

Tags [@DimStar77 ]

$ git range-diff shadow/master gh/tfix tfix 
1:  3ad184cc ! 1:  8e8a817a etc/pam.d/Makefile.am: Fix typo
    @@ Commit message
         Cc: David Runge <dvzrv@archlinux.org>
         Cc: Iker Pedrosa <ipedrosa@redhat.com>
         Co-developed-by: Dominique Leuenberger <dleuenberger@suse.com>
    +    Signed-off-by: Dominique Leuenberger <dleuenberger@suse.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## etc/pam.d/Makefile.am ##

alejandro-colomar commented 6 months ago

@DimStar77

Can you please confirm that this fixes the build on Suse?

jubalh commented 6 months ago

I pulled this PR into a patch and get:

Complete spec file

  --enable-shadowgrp \
  --enable-account-tools-setuid \
  --with-audit \
  --with-libpam \
  --with-sha-crypt \
  --with-acl \
  --with-attr \
  --with-nscd \
  --with-selinux \
  --without-libcrack \
  --without-libbsd \
  --with-group-name-max-length=32 \

Complete build log

[   71s] Making all in pam.d
[   71s] make[3]: *** No rule to make target 'chgpasswd', needed by 'all-am'.  Stop.

DimStar77 commented 6 months ago

I pulled this PR into a patch and get: [ 71s] Making all in pam.d [ 71s] make[3]: *** No rule to make target 'chgpasswd', needed by 'all-am'. Stop.

Patching the released 4.14.4 tarball with only this patch won't work - as the file https://github.com/shadow-maint/shadow/blob/master/etc/pam.d/chgpasswd is missing in the tarball (make dist had incomplete data)

you will have to add the content of that file to your build

jubalh commented 6 months ago

is missing in the tarball (make dist had incomplete data)

Ah of course. I thought I'm missing something here. Excuse me, I was doing this while being in a meeting and wanted to use the time "productive" ;) That obviously didn't work out :)

Everything builds fine now.

alejandro-colomar commented 6 months ago

v2c changes:

Tags [@jubalh , @ikerexxe ]

$ git range-diff shadow/master gh/tfix tfix 
1:  8e8a817a ! 1:  60d6b698 etc/pam.d/Makefile.am: Fix typo
    @@ Commit message
         Reported-by: Michael Vetter <jubalh@iodoru.org>
         Cc: loqs <https://github.com/loqs>
         Cc: David Runge <dvzrv@archlinux.org>
    -    Cc: Iker Pedrosa <ipedrosa@redhat.com>
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
    +    Tested-by: Michael Vetter <jubalh@iodoru.org>
    +    Reviewed-by: Michael Vetter <jubalh@iodoru.org>
         Co-developed-by: Dominique Leuenberger <dleuenberger@suse.com>
         Signed-off-by: Dominique Leuenberger <dleuenberger@suse.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

While I'd like if someone from Arch could have a look at this,

Versions 4.14.3 and older have been broken for Arch for a long time.
4.14.4 has introduced a regression for Gentoo (and possibly others), and is marked as broken, so no recent good version has ever worked for Arch correctly.
This fixes the regression for Gentoo (and possibly others).
We believe it probably won't break Arch, but we don't know for sure.

So, @ikerexxe , I'll let you merge. I think we can merge already, but maybe you want to give them some time to have a look.

loqs commented 6 months ago

This does not cause any change on Arch due to its use of --disable-account-tools-setuid ships chgpasswd without pam support.

alejandro-colomar commented 6 months ago

This does not cause any change on Arch due to its use of --disable-account-tools-setuid ships chgpasswd without pam support.

v2d changes:

Arch review by @loqs

$ git range-diff shadow/master gh/tfix tfix 
1:  60d6b698 ! 1:  96fc104a etc/pam.d/Makefile.am: Fix typo
    @@ Commit message
         Link: <https://github.com/shadow-maint/shadow/issues/926#issuecomment-1941324761>
         Reported-by: Dominique Leuenberger <dleuenberger@suse.com>
         Reported-by: Michael Vetter <jubalh@iodoru.org>
    -    Cc: loqs <https://github.com/loqs>
         Cc: David Runge <dvzrv@archlinux.org>
         Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Tested-by: Michael Vetter <jubalh@iodoru.org>
         Reviewed-by: Michael Vetter <jubalh@iodoru.org>
    +    Reviewed-by: loqs <https://github.com/loqs>
         Co-developed-by: Dominique Leuenberger <dleuenberger@suse.com>
         Signed-off-by: Dominique Leuenberger <dleuenberger@suse.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

Now I'll merge. :)

shadow-maint / shadow

etc/pam.d/Makefile.am: Fix typo #949