Gentoo Forums
Gentoo Forums
Gentoo Forums
Quick Search: in
my first ebuild - procexp-1.0.178 - please review
View unanswered posts
View posts from last 24 hours

 
Reply to topic    Gentoo Forums Forum Index Portage & Programming
View previous topic :: View next topic  
Author Message
paluszak
Apprentice
Apprentice


Joined: 28 Jun 2004
Posts: 265
Location: Warsaw, Poland

PostPosted: Sat Sep 29, 2012 3:00 pm    Post subject: my first ebuild - procexp-1.0.178 - please review Reply with quote

Hello,

I wrote my very first ebuild for a graphical top-like tool. I would be grateful if someone a tad more experienced have a look before I submit it to bugzilla. TIA.

J.

Code:
# Copyright 1999-2012 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Header: $

EAPI="3"

inherit eutils

DESCRIPTION="Process Explorer for Linux, inspired by a Windows process explorer made by Mark Russinovich."
HOMEPAGE="http://sourceforge.net/apps/mediawiki/procexp/index.php?title=Main_Page"
SRC_URI="mirror://sourceforge/project/procexp/sources_v1.0/${PN}_${PV}.orig.tar.gz"

LICENSE="GPL-2"
SLOT="0"
KEYWORDS="~amd64 ~x86"
IUSE="network"

PYTHON_DEPEND="2:2.6"
RESTRICT_PYTHON_ABIS="2.*"

inherit python

RDEPEND="dev-python/PyQt4 \
   dev-python/configobj \
   dev-python/pyqwt \
   network? ( sys-apps/ethtool )"

DEPEND="${RDEPEND}"

pkg_setup() {
   python_set_active_version 2
}

src_prepare() {
   mv -f procexp/icon.png procexp/procexp.png
   sed -e 's|Icon=/usr/share/icons/procexp/icon.png|Icon=/usr/share/pixmaps/procexp.png|' < procexp/procexp.desktop > ${T}/procexp.desktop
   rm -f procexp/procexp.desktop
   sed -e 's|python|python2|' < procexp/procexp.sh > ${T}/procexp.sh
   rm -f procexp/procexp.sh
   rm -f procexp/process_explorer.spec
   rm -f procexp/make_rpm.py
   find procexp/ -type d -name .svn -print0 | xargs -0 /bin/rm -rf
}

src_install() {
   doicon procexp/procexp.png
   rm -f procexp/procexp.png
   domenu ${T}/procexp.desktop
   mkdir ${D}/usr/bin
   mv -f ${T}/procexp.sh ${D}/usr/bin/
   chmod +x ${D}/usr/bin/procexp.sh
   mkdir ${D}/usr/lib
   mkdir ${D}/usr/lib/procexp
   cp -Rf ${WORKDIR}/procexp/* ${D}/usr/lib/procexp/
}

pkg_postinst() {
   python_mod_optimize ${ROOT}usr/lib/procexp/
   if use network; then
      elog Run as root to take advantage of network monitoring features \(via ethtool\).
   fi
}

pkg_postrm() {
   python_mod_cleanup ${ROOT}usr/lib/procexp/
}
Back to top
View user's profile Send private message
John R. Graham
Administrator
Administrator


Joined: 08 Mar 2005
Posts: 10587
Location: Somewhere over Atlanta, Georgia

PostPosted: Sat Sep 29, 2012 3:59 pm    Post subject: Reply with quote

Hi, @paluszak. Although you'll get help here too, for quicker feedback, you may want to contact the Gentoo Sunrise folks on IRC at #gentoo-sunrise. :wink:

- John
_________________
I can confirm that I have received between 0 and 499 National Security Letters.


Last edited by John R. Graham on Mon Oct 01, 2012 6:44 pm; edited 1 time in total
Back to top
View user's profile Send private message
phajdan.jr
Retired Dev
Retired Dev


Joined: 23 Mar 2006
Posts: 1777
Location: Poland

PostPosted: Sun Sep 30, 2012 8:59 am    Post subject: Re: my first ebuild - procexp-1.0.178 - please review Reply with quote

paluszak wrote:
Code:
EAPI="3"


nit: Why not EAPI="4"?

paluszak wrote:
Code:

src_prepare() {
   mv -f procexp/icon.png procexp/procexp.png
   sed -e 's|Icon=/usr/share/icons/procexp/icon.png|Icon=/usr/share/pixmaps/procexp.png|' < procexp/procexp.desktop > ${T}/procexp.desktop
   rm -f procexp/procexp.desktop
   sed -e 's|python|python2|' < procexp/procexp.sh > ${T}/procexp.sh
   rm -f procexp/procexp.sh
   rm -f procexp/process_explorer.spec
   rm -f procexp/make_rpm.py
   find procexp/ -type d -name .svn -print0 | xargs -0 /bin/rm -rf
}


You need || die after each of those commands. Instead of piping find's output to xargs, just tell find to delete things (or we might have code for that in some eclass).

Are you sure all this code in src_prepare() is needed? Have you considered contacting upstream to make their build system more distro-friendly?

paluszak wrote:
Code:

src_install() {
   doicon procexp/procexp.png
   rm -f procexp/procexp.png
   domenu ${T}/procexp.desktop
   mkdir ${D}/usr/bin
   mv -f ${T}/procexp.sh ${D}/usr/bin/
   chmod +x ${D}/usr/bin/procexp.sh
   mkdir ${D}/usr/lib
   mkdir ${D}/usr/lib/procexp
   cp -Rf ${WORKDIR}/procexp/* ${D}/usr/lib/procexp/
}


Same here, you need || die, and doesn't upstream provide make install?

paluszak wrote:
Code:

pkg_postinst() {
   python_mod_optimize ${ROOT}usr/lib/procexp/
   if use network; then
      elog Run as root to take advantage of network monitoring features \(via ethtool\).
   fi
}


Why don't you quote the elog message in double quotes?
_________________
http://phajdan-jr.blogspot.com/
Back to top
View user's profile Send private message
Hu
Moderator
Moderator


Joined: 06 Mar 2007
Posts: 21431

PostPosted: Sun Sep 30, 2012 3:36 pm    Post subject: Re: my first ebuild - procexp-1.0.178 - please review Reply with quote

phajdan.jr wrote:
paluszak wrote:
Code:
src_prepare() {
   rm -f procexp/procexp.desktop
   rm -f procexp/procexp.sh
   rm -f procexp/process_explorer.spec
   rm -f procexp/make_rpm.py
}
You need || die after each of those commands.
OP: beware that rm -f will silently exit success on attempts to remove a missing file, while rm without a -f will exit failure on attempts to remove a missing file. If you add the || die suggested by phajdan.jr, you may also want to remove the -f if you want to detect when the files do not exist.
Back to top
View user's profile Send private message
paluszak
Apprentice
Apprentice


Joined: 28 Jun 2004
Posts: 265
Location: Warsaw, Poland

PostPosted: Sun Sep 30, 2012 4:01 pm    Post subject: Re: my first ebuild - procexp-1.0.178 - please review Reply with quote

phajdan.jr wrote:
paluszak wrote:
Code:
EAPI="3"


nit: Why not EAPI="4"?
I updated it to EAPI=4.
Quote:
paluszak wrote:
Code:

src_prepare() {
   mv -f procexp/icon.png procexp/procexp.png
   sed -e 's|Icon=/usr/share/icons/procexp/icon.png|Icon=/usr/share/pixmaps/procexp.png|' < procexp/procexp.desktop > ${T}/procexp.desktop
   rm -f procexp/procexp.desktop
   sed -e 's|python|python2|' < procexp/procexp.sh > ${T}/procexp.sh
   rm -f procexp/procexp.sh
   rm -f procexp/process_explorer.spec
   rm -f procexp/make_rpm.py
   find procexp/ -type d -name .svn -print0 | xargs -0 /bin/rm -rf
}


You need || die after each of those commands. Instead of piping find's output to xargs, just tell find to delete things (or we might have code for that in some eclass).


I added || die and found escn_clean in eutils.
Quote:

Are you sure all this code in src_prepare() is needed? Have you considered contacting upstream to make their build system more distro-friendly?
Just a raw tarball and couple of debs and rpms on the project page. :(
Quote:

paluszak wrote:
Code:

src_install() {
   doicon procexp/procexp.png
   rm -f procexp/procexp.png
   domenu ${T}/procexp.desktop
   mkdir ${D}/usr/bin
   mv -f ${T}/procexp.sh ${D}/usr/bin/
   chmod +x ${D}/usr/bin/procexp.sh
   mkdir ${D}/usr/lib
   mkdir ${D}/usr/lib/procexp
   cp -Rf ${WORKDIR}/procexp/* ${D}/usr/lib/procexp/
}


Same here, you need || die, and doesn't upstream provide make install?
I added || die, for the second question, see above.
Quote:

paluszak wrote:
Code:

pkg_postinst() {
   python_mod_optimize ${ROOT}usr/lib/procexp/
   if use network; then
      elog Run as root to take advantage of network monitoring features \(via ethtool\).
   fi
}


Why don't you quote the elog message in double quotes?
Done.
Hu wrote:
phajdan.jr wrote:
paluszak wrote:
Code:
src_prepare() {
   rm -f procexp/procexp.desktop
   rm -f procexp/procexp.sh
   rm -f procexp/process_explorer.spec
   rm -f procexp/make_rpm.py
}
You need || die after each of those commands.
OP: beware that rm -f will silently exit success on attempts to remove a missing file, while rm without a -f will exit failure on attempts to remove a missing file. If you add the || die suggested by phajdan.jr, you may also want to remove the -f if you want to detect when the files do not exist.
Thanks, I removed the -f's, streamlined it a little bit (rm accepts more than one file to remove) and made it multilib-friendly.

Now it looks like this:
Code:
# Copyright 1999-2012 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Header: $

EAPI="4"
PYTHON_DEPEND="2:2.6"
RESTRICT_PYTHON_ABIS="2.*"

inherit eutils python multilib

DESCRIPTION="Process Explorer for Linux, inspired by a Windows process explorer made by Mark Russinovich."
HOMEPAGE="http://sourceforge.net/apps/mediawiki/procexp/index.php?title=Main_Page"
SRC_URI="mirror://sourceforge/project/procexp/sources_v1.0/${PN}_${PV}.orig.tar.gz"

LICENSE="GPL-2"
SLOT="0"
KEYWORDS="~amd64 ~x86"
IUSE="network"

RDEPEND="dev-python/PyQt4 \
   dev-python/configobj \
   dev-python/pyqwt \
   network? ( sys-apps/ethtool )"

src_unpack() {
   unpack ${A}
   mv procexp ${P} || die
}

pkg_setup() {
   python_set_active_version 2
}

src_prepare() {
   mv -f icon.png ${T}/procexp.png || die
   sed -e "s|Icon=/usr/share/icons/procexp/icon.png|Icon=/usr/share/pixmaps/procexp.png|" < procexp.desktop > ${T}/procexp.desktop || die
   sed -e "s|python /usr/lib/procexp/procexp.py|python2 /usr/$(get_libdir)/procexp/procexp.py|" < procexp.sh > ${T}/procexp.sh || die
   rm procexp.desktop procexp.sh process_explorer.spec make_rpm.py || die
   esvn_clean
}

src_install() {
   doicon ${T}/procexp.png
   domenu ${T}/procexp.desktop
   mkdir ${D}/usr/bin || die
   mv -f ${T}/procexp.sh ${D}/usr/bin/ || die
   chmod +x ${D}/usr/bin/procexp.sh || die
   if [ ! -d ${D}/usr/$(get_libdir) ]; then
      mkdir ${D}/usr/$(get_libdir) || die
   fi
   mkdir ${D}/usr/$(get_libdir)/${PN} || die
   cp -Rf ${WORKDIR}/${P}/* ${D}/usr/$(get_libdir)/${PN}/ || die
}

pkg_postinst() {
   python_mod_optimize ${ROOT}usr/$(get_libdir)/${PN}/
   if use network; then
      elog "Run as root to take advantage of network monitoring features (via ethtool)."
   fi
}

pkg_postrm() {
   python_mod_cleanup ${ROOT}usr/$(get_libdir)/${PN}/
}
Back to top
View user's profile Send private message
dol-sen
Retired Dev
Retired Dev


Joined: 30 Jun 2002
Posts: 2805
Location: Richmond, BC, Canada

PostPosted: Sun Sep 30, 2012 8:36 pm    Post subject: Reply with quote

I had a look at the source. It should have a setup.py and install itself into site-packages properly instead of on it's own. Also instead of the procexp.sh script, the procexp.py file can be installed into /usr/bin without the .py extension as long as it has the correct shebang.
Code:
#!/usr/bin/python
In that way it can properly be installed to multiple python versions if it supports it. As well, then python-updater would work for it too. The current install method, it would likely need manual updating, rebuilding.
_________________
Brian
Porthole, the Portage GUI frontend irc@freenode: #gentoo-guis, #porthole, Blog
layman, gentoolkit, CoreBuilder, esearch...
Back to top
View user's profile Send private message
steveL
Watchman
Watchman


Joined: 13 Sep 2006
Posts: 5153
Location: The Peanut Gallery

PostPosted: Sun Sep 30, 2012 11:11 pm    Post subject: Reply with quote

On a far more fundamental level, you must learn to quote parameter expansions.

unpack ${A} should be: unpack "${A}" or (more simply to read and write): unpack "$A". You only need the ${} construction when the next character in the script would otherwise be considered part of the variable name (so a letter, number or underscore), eg echo "${foo}bar". Or when you're doing manipulative expansions like echo "${foo#*bar}".

mv -f icon.png ${T}/procexp.png should be: mv -f icon.png "${T}/procexp.png"
mkdir ${D}/usr/bin should be: mkdir "${D}/usr/bin" and so on.

Otherwise, if there are spaces in the directory names, the shell will split the parameter into two or more arguments (at the spaces.) Additionally any * or ? will be matched as a wildcard character, as will anything between [ and ] in the string. (This last part about wildcarding is not so urgent here, but is still something you need to know about if shell-scripting.)
greybot wrote:

"USE MORE QUOTES!" They are vital. Also, learn the difference between ' and " and `. See http://mywiki.wooledge.org/Quotes and http://wiki.bash-hackers.org/syntax/words


http://www.grymoire.com/Unix/Quote.html is also good. Usually, you should quote every parameter expansion ($foo).

I would strongly recommend you join #bash on chat.freenode.org to learn more about portable bash scripting. It's worth it, believe me.
Back to top
View user's profile Send private message
phajdan.jr
Retired Dev
Retired Dev


Joined: 23 Mar 2006
Posts: 1777
Location: Poland

PostPosted: Mon Oct 01, 2012 8:08 am    Post subject: Re: my first ebuild - procexp-1.0.178 - please review Reply with quote

paluszak wrote:
Code:
src_unpack() {
   unpack ${A}
   mv procexp ${P} || die
}


You can just set $S instead.

steveL wrote:
unpack ${A} should be: unpack "${A}"


While your general comment about quoting is right, the above example is wrong. Do not quote $A! See http://devmanual.gentoo.org/ebuild-writing/functions/src_unpack/index.html . The reason is that A may contain multiple URLs, space-delimited.
_________________
http://phajdan-jr.blogspot.com/
Back to top
View user's profile Send private message
paluszak
Apprentice
Apprentice


Joined: 28 Jun 2004
Posts: 265
Location: Warsaw, Poland

PostPosted: Mon Oct 01, 2012 5:05 pm    Post subject: Reply with quote

dol-sen wrote:
I had a look at the source. It should have a setup.py and install itself into site-packages properly instead of on it's own. Also instead of the procexp.sh script, the procexp.py file can be installed into /usr/bin without the .py extension as long as it has the correct shebang.
Code:
#!/usr/bin/python
In that way it can properly be installed to multiple python versions if it supports it. As well, then python-updater would work for it too. The current install method, it would likely need manual updating, rebuilding.
1. Sadly, the upstream doesn't support any setup.py
2. The package supports only python 2:2.6 as defined in the ebuild.
3. Doesn't calling python_need_rebuild mark the package as needing a rebuild in case python-updater is called?
steveL wrote:
On a far more fundamental level, you must learn to quote parameter expansions.

unpack ${A} should be: unpack "${A}" or (more simply to read and write): unpack "$A". You only need the ${} construction when the next character in the script would otherwise be considered part of the variable name (so a letter, number or underscore), eg echo "${foo}bar". Or when you're doing manipulative expansions like echo "${foo#*bar}".

mv -f icon.png ${T}/procexp.png should be: mv -f icon.png "${T}/procexp.png"
mkdir ${D}/usr/bin should be: mkdir "${D}/usr/bin" and so on.

Otherwise, if there are spaces in the directory names, the shell will split the parameter into two or more arguments (at the spaces.) Additionally any * or ? will be matched as a wildcard character, as will anything between [ and ] in the string. (This last part about wildcarding is not so urgent here, but is still something you need to know about if shell-scripting.)
greybot wrote:

"USE MORE QUOTES!" They are vital. Also, learn the difference between ' and " and `. See http://mywiki.wooledge.org/Quotes and http://wiki.bash-hackers.org/syntax/words


http://www.grymoire.com/Unix/Quote.html is also good. Usually, you should quote every parameter expansion ($foo).

I would strongly recommend you join #bash on chat.freenode.org to learn more about portable bash scripting. It's worth it, believe me.
Thanks, I put the quotes around everything that might require it. And thanks for the links.
phajdan.jr wrote:
paluszak wrote:
Code:
src_unpack() {
   unpack ${A}
   mv procexp ${P} || die
}


You can just set $S instead.
I got the same idea after posting here.

So the ebuild now looks like this (after making it profix-friendly):
Code:
# Copyright 1999-2012 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Header: $

EAPI="4"
PYTHON_DEPEND="2:2.6"
RESTRICT_PYTHON_ABIS="2.*"

inherit eutils python multilib

DESCRIPTION="Process Explorer for Linux, inspired by a Windows process explorer made by Mark Russinovich."
HOMEPAGE="http://sourceforge.net/apps/mediawiki/procexp/index.php?title=Main_Page"
SRC_URI="mirror://sourceforge/project/procexp/sources_v1.0/${PN}_${PV}.orig.tar.gz"

LICENSE="GPL-2"
SLOT="0"
KEYWORDS="~amd64 ~x86"
IUSE="network"

RDEPEND="dev-python/PyQt4 \
   dev-python/configobj \
   dev-python/pyqwt \
   network? ( sys-apps/ethtool )"

S="${WORKDIR}/procexp"

pkg_setup() {
   python_set_active_version 2
   python_pkg_setup
}

src_prepare() {
   mv -f icon.png "${T}/procexp.png" || die
   sed -e "s|Icon=/usr/share/icons/procexp/icon.png|Icon=${EROOT}usr/share/pixmaps/procexp.png|" < procexp.desktop > "${T}/procexp.desktop" || die
   sed -e "s|python /usr/lib/procexp/procexp.py|python2 ${EROOT}usr/$(get_libdir)/procexp/procexp.py|" < procexp.sh > "${T}/procexp.sh" || die
   rm procexp.desktop procexp.sh process_explorer.spec make_rpm.py || die
   esvn_clean
}

src_install() {
   doicon "${T}/procexp.png"
   domenu "${T}/procexp.desktop"
   mkdir "${ED}/usr/bin" || die
   mv -f "${T}/procexp.sh" "${ED}/usr/bin/" || die
   chmod +x "${ED}/usr/bin/procexp.sh" || die
   if [ ! -d "${ED}/usr/$(get_libdir)" ]; then
      mkdir "${ED}/usr/$(get_libdir)" || die
   fi
   mkdir "${ED}/usr/$(get_libdir)/${PN}" || die
   cp -Rf "${S}/"* "${ED}/usr/$(get_libdir)/${PN}/" || die
}

pkg_postinst() {
   python_need_rebuild
   python_mod_optimize "${EROOT}usr/$(get_libdir)/${PN}/"
   if use network; then
      elog "Run as root to take advantage of network monitoring features (via ethtool)."
   fi
}

pkg_postrm() {
   python_mod_cleanup "${EROOT}usr/$(get_libdir)/${PN}/"
}
Back to top
View user's profile Send private message
steveL
Watchman
Watchman


Joined: 13 Sep 2006
Posts: 5153
Location: The Peanut Gallery

PostPosted: Tue Oct 02, 2012 6:39 am    Post subject: Re: my first ebuild - procexp-1.0.178 - please review Reply with quote

phajdan.jr wrote:
While your general comment about quoting is right, the above example is wrong. Do not quote $A! See http://devmanual.gentoo.org/ebuild-writing/functions/src_unpack/index.html . The reason is that A may contain multiple URLs, space-delimited.

Ah my bad, thanks for picking up on it.
Back to top
View user's profile Send private message
phajdan.jr
Retired Dev
Retired Dev


Joined: 23 Mar 2006
Posts: 1777
Location: Poland

PostPosted: Tue Oct 02, 2012 10:55 am    Post subject: Reply with quote

I see the ebuild has quite improved now, nice progress. :wink:

paluszak wrote:
1. Sadly, the upstream doesn't support any setup.py


Thanks for checking. However, I insist on asking upstream to change that. One part of high-quality packaging (it's essential, you seriously cannot do without it, except truly trivial cases) is good cooperation with upstream (that includes sending them patches etc). By the way, it also makes the ebuild maintainable - more code in upstream's build system, less in the ebuild etc.

paluszak wrote:
2. The package supports only python 2:2.6 as defined in the ebuild.


That's fine IMO.

paluszak wrote:
Code:
S="${WORKDIR}/procexp"


I think you could use $PN above.

paluszak wrote:
Code:
mv -f icon.png "${T}/procexp.png" || die


Is the -f above necessary?

paluszak wrote:
Code:

   sed -e "s|Icon=/usr/share/icons/procexp/icon.png|Icon=${EROOT}usr/share/pixmaps/procexp.png|" < procexp.desktop > "${T}/procexp.desktop" || die
   sed -e "s|python /usr/lib/procexp/procexp.py|python2 ${EROOT}usr/$(get_libdir)/procexp/procexp.py|" < procexp.sh > "${T}/procexp.sh" || die


Are those sed calls truly necessary for the package to work or for other important reasons? Are you aware of sed's --in-place (-i) option?

paluszak wrote:
Code:

   rm procexp.desktop procexp.sh process_explorer.spec make_rpm.py || die


Consider adding a comment above to explain why the files are being removed.

paluszak wrote:
Code:

src_install() {
   doicon "${T}/procexp.png"
   domenu "${T}/procexp.desktop"
   mkdir "${ED}/usr/bin" || die
   mv -f "${T}/procexp.sh" "${ED}/usr/bin/" || die
   chmod +x "${ED}/usr/bin/procexp.sh" || die
   if [ ! -d "${ED}/usr/$(get_libdir)" ]; then
      mkdir "${ED}/usr/$(get_libdir)" || die
   fi
   mkdir "${ED}/usr/$(get_libdir)/${PN}" || die
   cp -Rf "${S}/"* "${ED}/usr/$(get_libdir)/${PN}/" || die
}


-f option for mv and cp shouldn't be needed. Similarly, you shouldn't need the "if" - the staging directory is initially empty, so you should know what's there. Also, it's better to use "mkdir -p". There are also useful helpers like dodir, newdir, doins, insinto, etc.
_________________
http://phajdan-jr.blogspot.com/
Back to top
View user's profile Send private message
paluszak
Apprentice
Apprentice


Joined: 28 Jun 2004
Posts: 265
Location: Warsaw, Poland

PostPosted: Tue Oct 02, 2012 12:42 pm    Post subject: Reply with quote

phajdan.jr wrote:
I see the ebuild has quite improved now, nice progress. :wink:


Thanks.

phajdan.jr wrote:
paluszak wrote:
1. Sadly, the upstream doesn't support any setup.py


Thanks for checking. However, I insist on asking upstream to change that. One part of high-quality packaging (it's essential, you seriously cannot do without it, except truly trivial cases) is good cooperation with upstream (that includes sending them patches etc). By the way, it also makes the ebuild maintainable - more code in upstream's build system, less in the ebuild etc.


I opened an artifact on the project's tracker.

phajdan.jr wrote:
paluszak wrote:
2. The package supports only python 2:2.6 as defined in the ebuild.


That's fine IMO.

paluszak wrote:
Code:
S="${WORKDIR}/procexp"


I think you could use $PN above.
Makes sense
phajdan.jr wrote:
paluszak wrote:
Code:
mv -f icon.png "${T}/procexp.png" || die


Is the -f above necessary?
Not really. I got rid of all mv's anyway.
phajdan.jr wrote:
paluszak wrote:
Code:

   sed -e "s|Icon=/usr/share/icons/procexp/icon.png|Icon=${EROOT}usr/share/pixmaps/procexp.png|" < procexp.desktop > "${T}/procexp.desktop" || die
   sed -e "s|python /usr/lib/procexp/procexp.py|python2 ${EROOT}usr/$(get_libdir)/procexp/procexp.py|" < procexp.sh > "${T}/procexp.sh" || die


Are those sed calls truly necessary for the package to work or for other important reasons? Are you aware of sed's --in-place (-i) option?
1. I got rid of the unnecessary shell script. Adding a shebang, setting executable bit abd creating a symlink seem more elegant to me.
2. sed'ing procexp.desktop is not necessary per se, but without correct paths in *.desktop a menu entry would be broken.
3. I forgot about it sed -i. :oops:
phajdan.jr wrote:
paluszak wrote:
Code:

   rm procexp.desktop procexp.sh process_explorer.spec make_rpm.py || die


Consider adding a comment above to explain why the files are being removed.
Added some explanation.
phajdan.jr wrote:
paluszak wrote:
Code:

src_install() {
   doicon "${T}/procexp.png"
   domenu "${T}/procexp.desktop"
   mkdir "${ED}/usr/bin" || die
   mv -f "${T}/procexp.sh" "${ED}/usr/bin/" || die
   chmod +x "${ED}/usr/bin/procexp.sh" || die
   if [ ! -d "${ED}/usr/$(get_libdir)" ]; then
      mkdir "${ED}/usr/$(get_libdir)" || die
   fi
   mkdir "${ED}/usr/$(get_libdir)/${PN}" || die
   cp -Rf "${S}/"* "${ED}/usr/$(get_libdir)/${PN}/" || die
}


-f option for mv and cp shouldn't be needed. Similarly, you shouldn't need the "if" - the staging directory is initially empty, so you should know what's there. Also, it's better to use "mkdir -p". There are also useful helpers like dodir, newdir, doins, insinto, etc.
I changed everything into doins, doicon, dodir etc, got rid of if/fi and a direct call to cp. Now it looks like this:
Code:
# Copyright 1999-2012 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Header: $

EAPI="4"
PYTHON_DEPEND="2:2.6"
RESTRICT_PYTHON_ABIS="2.*"

inherit eutils python multilib

DESCRIPTION="Process Explorer for Linux, inspired by a Windows process explorer made by Mark Russinovich."
HOMEPAGE="http://sourceforge.net/apps/mediawiki/procexp/index.php?title=Main_Page"
SRC_URI="mirror://sourceforge/project/procexp/sources_v1.0/${PN}_${PV}.orig.tar.gz"

LICENSE="GPL-2"
SLOT="0"
KEYWORDS="~amd64 ~x86"
IUSE="network"

RDEPEND="dev-python/PyQt4 \
   dev-python/configobj \
   dev-python/pyqwt \
   network? ( sys-apps/ethtool )"

S="${WORKDIR}/${PN}"

pkg_setup() {
   python_set_active_version 2
   python_pkg_setup
}

src_prepare() {
   einfo "Setting correct paths and adding shebang..."
   sed -e "s|Icon=/usr/share/icons/procexp/icon.png|Icon=${EROOT}usr/share/pixmaps/procexp.png|" \
      -e "s|Exec=procexp.sh|Exec=procexp|" -i procexp.desktop || die
   sed -e '1i#!/usr/bin/env python2' -i procexp.py || die
   einfo "Removing unnecessary svn cruft and packaging system..."
   esvn_clean
   rm procexp.sh make_rpm.py process_explorer.spec || die
}

src_install() {
   newicon icon.png procexp.png
   rm icon.png || die
   domenu procexp.desktop
   rm procexp.desktop || die
   dodir "/usr/$(get_libdir)/${PN}"
   insinto "/usr/$(get_libdir)/${PN}/"
   doins -r *
   dodir /usr/bin
   dosym "${EROOT}usr/$(get_libdir)/${PN}/procexp.py" /usr/bin/procexp
   chmod +x "${ED}usr/$(get_libdir)/${PN}/procexp.py" || die
}

pkg_postinst() {
   python_need_rebuild
   python_mod_optimize "${EROOT}usr/$(get_libdir)/${PN}/"
   if use network; then
      elog "Run as root to take advantage of network monitoring features (via ethtool)."
   fi
}

pkg_postrm() {
   python_mod_cleanup "${EROOT}usr/$(get_libdir)/${PN}/"
}
Back to top
View user's profile Send private message
phajdan.jr
Retired Dev
Retired Dev


Joined: 23 Mar 2006
Posts: 1777
Location: Poland

PostPosted: Tue Oct 02, 2012 2:14 pm    Post subject: Reply with quote

paluszak wrote:
Code:

RDEPEND="dev-python/PyQt4 \
   dev-python/configobj \
   dev-python/pyqwt \
   network? ( sys-apps/ethtool )"


Those backslashes ("\") inside variables are not really needed and should be removed.

paluszak wrote:
Code:

   einfo "Setting correct paths and adding shebang..."
   einfo "Removing unnecessary svn cruft and packaging system..."


The above lines are not really needed unless the process takes noticeably long (e.g. few seconds) on a reasonably fast machine. In my earlier feedback I meant adding a shell comment (starting with #), not einfo.

paluszak wrote:
Code:

   sed -e '1i#!/usr/bin/env python2' -i procexp.py || die


There should be some function to handle the shebang in the python eclass.

Ah, and ideally put the link to upstream bug report about the build system in a comment inside the ebuild.
_________________
http://phajdan-jr.blogspot.com/
Back to top
View user's profile Send private message
paluszak
Apprentice
Apprentice


Joined: 28 Jun 2004
Posts: 265
Location: Warsaw, Poland

PostPosted: Tue Oct 02, 2012 3:00 pm    Post subject: Reply with quote

phajdan.jr wrote:
paluszak wrote:
Code:

RDEPEND="dev-python/PyQt4 \
   dev-python/configobj \
   dev-python/pyqwt \
   network? ( sys-apps/ethtool )"


Those backslashes ("\") inside variables are not really needed and should be removed.
Yep, I saw it somewhere and put it here without thinking. Removed.
phajdan.jr wrote:
paluszak wrote:
Code:

   einfo "Setting correct paths and adding shebang..."
   einfo "Removing unnecessary svn cruft and packaging system..."


The above lines are not really needed unless the process takes noticeably long (e.g. few seconds) on a reasonably fast machine. In my earlier feedback I meant adding a shell comment (starting with #), not einfo.
Ok.
phajdan.jr wrote:
paluszak wrote:
Code:

   sed -e '1i#!/usr/bin/env python2' -i procexp.py || die


There should be some function to handle the shebang in the python eclass.
As far as I could find there's only a function to convert shebangs (python_convert_shebangs). However, there's no shebang at all in the executable python file provided by upstream.
phajdan.jr wrote:
Ah, and ideally put the link to upstream bug report about the build system in a comment inside the ebuild.
Code:
# Copyright 1999-2012 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Header: $

EAPI="4"
PYTHON_DEPEND="2:2.6"
RESTRICT_PYTHON_ABIS="2.*"

inherit eutils python multilib

DESCRIPTION="Process Explorer for Linux, inspired by a Windows process explorer made by Mark Russinovich."
HOMEPAGE="http://sourceforge.net/apps/mediawiki/procexp/index.php?title=Main_Page"
SRC_URI="mirror://sourceforge/project/procexp/sources_v1.0/${PN}_${PV}.orig.tar.gz"

LICENSE="GPL-2"
SLOT="0"
KEYWORDS="~amd64 ~x86"
IUSE="network"

RDEPEND="dev-python/PyQt4
   dev-python/configobj
   dev-python/pyqwt
   network? ( sys-apps/ethtool )"

S="${WORKDIR}/${PN}"

# No build system provided by upstream, bug filed: https://sourceforge.net/tracker/?func=detail&aid=3573774&group_id=309156&atid=1301952

pkg_setup() {
   python_set_active_version 2
   python_pkg_setup
}

src_prepare() {
   # Setting correct paths and adding shebang.
   sed -e "s|Icon=/usr/share/icons/procexp/icon.png|Icon=${EROOT}usr/share/pixmaps/procexp.png|" \
      -e "s|Exec=procexp.sh|Exec=procexp|" -i procexp.desktop || die
   sed -e '1i#!/usr/bin/env python2' -i procexp.py || die
   # Removing unnecessary svn cruft, packaging system and shell script.
   esvn_clean
   rm procexp.sh make_rpm.py process_explorer.spec || die
}

src_install() {
   newicon icon.png procexp.png
   rm icon.png || die
   domenu procexp.desktop
   rm procexp.desktop || die
   dodir "/usr/$(get_libdir)/${PN}"
   insinto "/usr/$(get_libdir)/${PN}/"
   doins -r *
   dodir /usr/bin
   dosym "${EROOT}usr/$(get_libdir)/${PN}/procexp.py" /usr/bin/procexp
   chmod +x "${ED}usr/$(get_libdir)/${PN}/procexp.py" || die
}

pkg_postinst() {
   python_need_rebuild
   python_mod_optimize "${EROOT}usr/$(get_libdir)/${PN}/"
   if use network; then
      elog "Run as root to take advantage of network monitoring features (via ethtool)."
   fi
}

pkg_postrm() {
   python_mod_cleanup "${EROOT}usr/$(get_libdir)/${PN}/"
}
Back to top
View user's profile Send private message
phajdan.jr
Retired Dev
Retired Dev


Joined: 23 Mar 2006
Posts: 1777
Location: Poland

PostPosted: Wed Oct 03, 2012 7:52 am    Post subject: Reply with quote

LGTM (looks good to me) :D
_________________
http://phajdan-jr.blogspot.com/
Back to top
View user's profile Send private message
paluszak
Apprentice
Apprentice


Joined: 28 Jun 2004
Posts: 265
Location: Warsaw, Poland

PostPosted: Wed Oct 03, 2012 2:04 pm    Post subject: Reply with quote

phajdan.jr wrote:
LGTM (looks good to me) :D
I submitted a bug on bugzilla: https://bugs.gentoo.org/show_bug.cgi?id=437072 Maybe I should try to have it added to Sunrise?
Back to top
View user's profile Send private message
John R. Graham
Administrator
Administrator


Joined: 08 Mar 2005
Posts: 10587
Location: Somewhere over Atlanta, Georgia

PostPosted: Wed Oct 03, 2012 2:29 pm    Post subject: Reply with quote

If no developer is initially interested in maintaining it, that's a good route. Doesn't hurt to do it anyway and makes it available to others through a standard mechanism: layman and the the sunrise overlay.

- John
_________________
I can confirm that I have received between 0 and 499 National Security Letters.
Back to top
View user's profile Send private message
paluszak
Apprentice
Apprentice


Joined: 28 Jun 2004
Posts: 265
Location: Warsaw, Poland

PostPosted: Wed Oct 03, 2012 3:28 pm    Post subject: Reply with quote

Guys on #gentoo-sunrise gave me some additional tips and the latest version looks like this:
Code:
# Copyright 1999-2012 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Header: $

EAPI="4"
PYTHON_DEPEND="2:2.6"

inherit eutils python multilib gnome2-utils

DESCRIPTION="Process Explorer for Linux, inspired by a Windows process explorer made by Mark Russinovich"
HOMEPAGE="http://sourceforge.net/apps/mediawiki/procexp/index.php?title=Main_Page"
SRC_URI="mirror://sourceforge/project/procexp/sources_v1.0/${PN}_${PV}.orig.tar.gz"

LICENSE="GPL-2"
SLOT="0"
KEYWORDS="~amd64 ~x86"
IUSE=""

RDEPEND="dev-python/PyQt4
   dev-python/configobj
   dev-python/pyqwt"

S="${WORKDIR}/${PN}"

# No build system provided by upstream, bug filed: https://sourceforge.net/tracker/?func=detail&aid=3573774&group_id=309156&atid=1301952

pkg_setup() {
   python_set_active_version 2
   python_pkg_setup
}

src_prepare() {
   # Adding shebang.
   sed -e '1i#!/usr/bin/env python2' -i procexp.py || die
   # Removing unnecessary svn cruft, packaging system and shell script.
   esvn_clean
   rm procexp.desktop procexp.sh make_rpm.py process_explorer.spec || die
}

src_install() {
   newicon -s 48 icon.png procexp.png
   rm icon.png || die
   make_desktop_entry ${PN} "Linux Process Explorer" /usr/share/icons/hicolor/48x48/apps/${PN}.png "System;Utility;"
   dodir "/usr/$(get_libdir)/${PN}"
   insinto "/usr/$(get_libdir)/${PN}/"
   doins -r *
   dodir /usr/bin
   dosym "${EROOT}usr/$(get_libdir)/${PN}/procexp.py" /usr/bin/procexp
   fperms +x "/usr/$(get_libdir)/${PN}/procexp.py" || die
}

pkg_preinst() {
   gnome2_icon_savelist
}

pkg_postinst() {
   python_need_rebuild
   python_mod_optimize "${EROOT}usr/$(get_libdir)/${PN}/"
   gnome2_icon_cache_update
   elog "Emerge sys-apps/ethtool and run as root to take advantage of network monitoring features (via ethtool)."
}

pkg_postrm() {
   python_mod_cleanup "${EROOT}usr/$(get_libdir)/${PN}/"
   gnome2_icon_cache_update
}
Back to top
View user's profile Send private message
Display posts from previous:   
Reply to topic    Gentoo Forums Forum Index Portage & Programming All times are GMT
Page 1 of 1

 
Jump to:  
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum