base-files: move shells generating process to pkg_postinst

PATCH Superseded Ming Liu 3 participants 8 messages Oct 17, 2013

Share & tools

Ming Liu

author

2013-10-17 10:03 UTC #0 lore raw

PATCH v2 1/2 base-files: move shells generating process to pkg_postinst

The "shells" file lists several valid login shells, of which some might be not present in a particular rootfs, this is unreasonable.

Fix it by moving "shells" file generating process to pkg_postinst, for other shells must do the same thing to register themselves to this file.

Signed-off-by Ming Liu <[email protected]>

---
 meta/recipes-core/base-files/base-files/shells    | 8 --------
 meta/recipes-core/base-files/base-files_3.0.14.bb | 8 ++++++--
 2 files changed, 6 insertions(+), 10 deletions(-)
 delete mode 100644 meta/recipes-core/base-files/base-files/shells

meta/recipes-core/base-files/base-files/shells deleted +0 -8

deleted file mode 100644
index ce39b3d..0000000
@@ -1,8 +0,0 @@
-# /etc/shells: valid login shells
-/bin/sh
-/bin/ash
-/bin/bash
-/bin/dash
-/bin/ksh
-/usr/bin/ksh
-/usr/bin/screen

meta/recipes-core/base-files/base-files_3.0.14.bb +6 -2

index 054fefa..a169117 100644
@@ -16,7 +16,6 @@ SRC_URI = "file://rotation \
            file://inputrc \
            file://host.conf \
            file://profile \
-           file://shells \
            file://fstab \
            file://filesystems \
            file://issue.net \

@@ -92,7 +91,6 @@ do_install () {
 	install -m 0644 ${WORKDIR}/usbd ${D}${sysconfdir}/default/usbd
 	sed -i "s#ROOTHOME#${ROOT_HOME}#" ${WORKDIR}/profile
 	install -m 0644 ${WORKDIR}/profile ${D}${sysconfdir}/profile
-	install -m 0644 ${WORKDIR}/shells ${D}${sysconfdir}/shells
 	install -m 0755 ${WORKDIR}/share/dot.profile ${D}${sysconfdir}/skel/.profile
 	install -m 0755 ${WORKDIR}/share/dot.bashrc ${D}${sysconfdir}/skel/.bashrc
 	install -m 0644 ${WORKDIR}/inputrc ${D}${sysconfdir}/inputrc

@@ -137,6 +135,12 @@ do_install_append_linuxstdbase() {
         done
 }
 
+pkg_postinst_${PN} () {
+        touch $D${sysconfdir}/shells
+        grep -q "^/bin/sh$" $D${sysconfdir}/shells || echo /bin/sh >> $D${sysconfdir}/shells
+}
+
+
 PACKAGES = "${PN}-doc ${PN} ${PN}-dev ${PN}-dbg"
 FILES_${PN} = "/"
 FILES_${PN}-doc = "${docdir} ${datadir}/common-licenses"

Signature · 1 line

1.8.3.3

Ming Liu

author

2013-10-17 10:03 UTC #1 lore raw

PATCH v2 2/2 screen: add pkg_postinst to register entry to /etc/shells

Signed-off-by Ming Liu <[email protected]>

---
 meta/recipes-extended/screen/screen_4.0.3.bb | 5 +++++
 1 file changed, 5 insertions(+)

meta/recipes-extended/screen/screen_4.0.3.bb +5 -0

index 1a7eb20..81e418b 100644
@@ -41,3 +41,8 @@ do_install_append () {
 		fi
 	done
 }
+
+pkg_postinst_${PN} () {
+	touch $D${sysconfdir}/shells
+	grep -q "${bindir}/screen" $D${sysconfdir}/shells || echo ${bindir}/screen >> $D${sysconfdir}/shells
+}

Signature · 1 line

1.8.3.3

Phil Blundell

2013-10-17 10:11 UTC #2 lore raw

Re: [PATCH V2 2/2] screen: add pkg_postinst to register entry to /etc/shells

meta/recipes-extended/screen/screen_4.0.3.bb do_install_append () { · line 48

Quoted 4 lines · Ming Liu

meta/recipes-extended/screen/screen_4.0.3.bb +4 -0

+pkg_postinst_${PN} () {
+	touch $D${sysconfdir}/shells
+	grep -q "${bindir}/screen" $D${sysconfdir}/shells || echo ${bindir}/screen >> $D${sysconfdir}/shells
+}

Is that "touch" really desirable? It seems as though screen ought not really to be creating /etc/shells if it isn't there already, and probably ought not to be updating the timestamp if it isn't actually changing the file either.

Ming Liu

author

2013-10-17 10:28 UTC #3 lore raw

Re: [PATCH V2 2/2] screen: add pkg_postinst to register entry to /etc/shells

Quoted 7 lines · Phil Blundell

meta/recipes-extended/screen/screen_4.0.3.bb do_install_append () { · line 48
Quoted 4 lines · Ming Liu
meta/recipes-extended/screen/screen_4.0.3.bb +4 -0
+pkg_postinst_${PN} () {
+	touch $D${sysconfdir}/shells
+	grep -q "${bindir}/screen" $D${sysconfdir}/shells || echo ${bindir}/screen >> $D${sysconfdir}/shells
+}
Is that "touch" really desirable? It seems as though screen ought not really to be creating /etc/shells if it isn't there already,

No, it's supposed to be created if /etc/shells doesn't exist already, otherwise, how could we echo text to it?

Quoted 3 lines

and
probably ought not to be updating the timestamp if it isn't actually changing the file either.

Yes, it may happen that the timestamp be updated even the it isn't actually changing the file, but only for upgrade case, and I think it's not a big deal. Actually I copied this piece of script from bash recipe, it's doing the same thing there.

//Ming Liu

Quoted 5 lines

p.

Phil Blundell

2013-10-17 10:53 UTC #4 lore raw

Re: [PATCH V2 2/2] screen: add pkg_postinst to register entry to /etc/shells

Quoted 10 lines · Ming Liu

Quoted 7 lines · Phil Blundell
meta/recipes-extended/screen/screen_4.0.3.bb do_install_append () { · line 48
Quoted 4 lines · Ming Liu
meta/recipes-extended/screen/screen_4.0.3.bb +4 -0
+pkg_postinst_${PN} () {
+	touch $D${sysconfdir}/shells
+	grep -q "${bindir}/screen" $D${sysconfdir}/shells || echo ${bindir}/screen >> $D${sysconfdir}/shells
+}
Is that "touch" really desirable? It seems as though screen ought not really to be creating /etc/shells if it isn't there already,
No, it's supposed to be created if /etc/shells doesn't exist already, otherwise, how could we echo text to it?

I guess there are two things to say about this:

1) not everybody wants /etc/shells, and having screen (or any other recipe) create that file unilaterally seems like a bad thing. Especially since there doesn't seem to be any corresponding postrm to clear it up if screen is then removed again, nor is /etc/shells mentioned in CONFFILES.

2) even if you did want to create /etc/shells, the touch is unnecessary because "echo foo >> FILE" is perfectly capable of creating the FILE if it didn't already exist.

Ming Liu

author

2013-10-17 11:06 UTC #5 lore raw

Re: [PATCH V2 2/2] screen: add pkg_postinst to register entry to /etc/shells

Quoted 19 lines · Phil Blundell

Quoted 10 lines · Ming Liu
Quoted 7 lines · Phil Blundell
meta/recipes-extended/screen/screen_4.0.3.bb do_install_append () { · line 48
Quoted 4 lines · Ming Liu
meta/recipes-extended/screen/screen_4.0.3.bb +4 -0
+pkg_postinst_${PN} () {
+	touch $D${sysconfdir}/shells
+	grep -q "${bindir}/screen" $D${sysconfdir}/shells || echo ${bindir}/screen >> $D${sysconfdir}/shells
+}
Is that "touch" really desirable? It seems as though screen ought not really to be creating /etc/shells if it isn't there already,
No, it's supposed to be created if /etc/shells doesn't exist already, otherwise, how could we echo text to it?
I guess there are two things to say about this:
1) not everybody wants /etc/shells, and having screen (or any other recipe) create that file unilaterally seems like a bad thing. Especially since there doesn't seem to be any corresponding postrm to clear it up if screen is then removed again, nor is /etc/shells mentioned in CONFFILES.

Quoted 3 lines

2) even if you did want to create /etc/shells, the touch is unnecessary because "echo foo >> FILE" is perfectly capable of creating the FILE if it didn't already exist.

Yes, you are correct for both of the two points, I will send the V3, includes the changes:
1 add postrm script, removing the entry in /etc/shells.
2 rm the unnecessary 'touch'.
3 change the regex for screen and bash recipes.(As Martin pointed out)

Anything else?

//Ming Liu

Quoted 6 lines

p.

Martin Jansa

2013-10-17 10:33 UTC #6 lore raw

Re: [PATCH V2 2/2] screen: add pkg_postinst to register entry to /etc/shells

In reply to Ming Liu

Quoted 10 lines · Ming Liu

Signed-off-by Ming Liu <[email protected]>
---
 meta/recipes-extended/screen/screen_4.0.3.bb | 5 +++++
 1 file changed, 5 insertions(+)
meta/recipes-extended/screen/screen_4.0.3.bb +0 -0
index 1a7eb20..81e418b 100644

meta/recipes-extended/screen/screen_4.0.3.bb do_install_append () { · line 47

Quoted 7 lines

meta/recipes-extended/screen/screen_4.0.3.bb +4 -0

@@ -41,3 +41,8 @@ do_install_append () {
 		fi
 	done
 }
+
+pkg_postinst_${PN} () {
+	touch $D${sysconfdir}/shells
+	grep -q "${bindir}/screen" $D${sysconfdir}/shells || echo ${bindir}/screen >> $D${sysconfdir}/shells

^ the same, we don't want to match /usr/bin/screenie
if it's there and "screen" isn't

Is there patch to add this postinst to bash and busybox?

Quoted 8 lines

meta/recipes-extended/screen/screen_4.0.3.bb +1 -0

+}

Signature · 6 lines

1.8.3.3

_______________________________________________
Openembedded-core mailing list
[email protected]
http://lists.openembedded.org/mailman/listinfo/openembedded-core

Signature · 1 line

Martin 'JaMa' Jansa     jabber: [email protected]

Ming Liu

author

2013-10-17 10:45 UTC #7 lore raw

Re: [PATCH V2 2/2] screen: add pkg_postinst to register entry to /etc/shells

Quoted 6 lines · Martin Jansa

Quoted 5 lines · Ming Liu
Signed-off-by Ming Liu <[email protected]>
---
  meta/recipes-extended/screen/screen_4.0.3.bb | 5 +++++
  1 file changed, 5 insertions(+)

Quoted 14 lines

meta/recipes-extended/screen/screen_4.0.3.bb do_install_append () { · line 47
Quoted 12 lines
meta/recipes-extended/screen/screen_4.0.3.bb +4 -0
index 1a7eb20..81e418b 100644
@@ -41,3 +41,8 @@ do_install_append () {
  		fi
  	done
  }
+
+pkg_postinst_${PN} () {
+	touch $D${sysconfdir}/shells
+	grep -q "${bindir}/screen" $D${sysconfdir}/shells || echo ${bindir}/screen >> $D${sysconfdir}/shells
^ the same, we don't want to match /usr/bin/screenie
if it's there and "screen" isn't

Yes, you are correct, I am too less comprehensive.

Quoted 2 lines

Is there patch to add this postinst to bash and busybox?

For bash, it already has it, as following:
------------------

pkg_postinst_${PN} () {
        touch $D${sysconfdir}/shells
        grep -q "bin/bash" $D${sysconfdir}/shells || echo /bin/bash >> 
$D${sysconfdir}/shells
        grep -q "bin/sh" $D${sysconfdir}/shells || echo /bin/sh >>

$D${sysconfdir}/shells
}
-----------------

But as you pointed out, it needs to be more stringent too.

For busybox, I don't think it need this script for it provides the symbol link: /bin/sh.

//Ming Liu

Quoted 9 lines

Quoted 9 lines
meta/recipes-extended/screen/screen_4.0.3.bb +1 -0
+}
Signature · 6 lines
1.8.3.3

_______________________________________________
Openembedded-core mailing list
[email protected]
http://lists.openembedded.org/mailman/listinfo/openembedded-core