-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
net-p2p/classified-ads: version bump to 0.13 #9449
Conversation
As I mentioned in IRC, there should be an empty line separating the body/description and the 'Closes' tag (which might be why the bot is not seeing it, though I'm not entirely sure). |
You'll probably want to change it in the commit message, the pull request description doen't matter that much. :] |
EAPI=7 for the new upstream version. Patches of 0.12 removed because upstream has changes included. Closes: https://bugs.gentoo.org/661338 Package-Manager: Portage-2.3.40, Repoman-2.3.9
4998a86
to
37283bd
Compare
Pull Request assignment Areas affected: ebuilds net-p2p/classified-ads: @operatornormal, @gentoo/proxy-maint Bugs linked: 661338 In order to force reassignment and/or bug reference scan, please append |
|
||
DESCRIPTION="Program for displaying classified advertisement items" | ||
HOMEPAGE="http://katiska.org/classified-ads/" | ||
SRC_URI="https://github.com/operatornormal/classified-ads/archive/${PV}.tar.gz \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No backslashes here, those variables work fine as multi-line.
https://github.com/operatornormal/classified-ads/blob/graphics/preprocessed.tar.gz?raw=true \ | ||
-> classified-ads-graphics-${PV}.tar.gz" | ||
|
||
LICENSE="LGPL-2.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either version 2.1 of the License, or (at your option) any later version.
So LGPL-2.1+
.
IUSE="doc test" | ||
|
||
RDEPEND="dev-libs/openssl:0 | ||
>=net-libs/libnatpmp-20130911 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use single tab for indent, please.
RDEPEND="dev-libs/openssl:0 | ||
>=net-libs/libnatpmp-20130911 | ||
<=net-libs/libnatpmp-20140401-r1 | ||
>=net-libs/miniupnpc-1.8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you link to the library, :=
to ensure your package gets rebuilt on ABI changes.
|
||
RDEPEND="dev-libs/openssl:0 | ||
>=net-libs/libnatpmp-20130911 | ||
<=net-libs/libnatpmp-20140401-r1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why require such an old version? :-(
dev-qt/qtprintsupport:5 | ||
media-libs/opus | ||
virtual/libintl | ||
dev-lang/tcl:= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please sort dependencies lexically.
popd > /dev/null || die | ||
fi | ||
if use test; then | ||
pushd testca > /dev/null || die |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emake -C testca
|
||
src_install() { | ||
docompress -x /usr/share/doc/ | ||
emake install INSTALL_ROOT="${D}" DESTDIR="${D}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need to use INSTALL_ROOT
and DESTDIR
simultaneously? oO
src_install() { | ||
docompress -x /usr/share/doc/ | ||
emake install INSTALL_ROOT="${D}" DESTDIR="${D}" | ||
use doc && dodoc -r doc/doxygen.generated/html/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finish it with /.
. Trailing slash is undefined behavior.
EAPI=7 | ||
PLOCALES="en fi sv da uk es" | ||
PLOCALE_BACKUP="en" | ||
inherit qmake-utils virtualx l10n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not using l10n at all here (and you aren't supposed to), so please remove that and PLOCALE*
.
Modifications to ebuild requested in comments of PR 9449. Closes: https://bugs.gentoo.org/661338 Package-Manager: Portage-2.3.40, Repoman-2.3.9
Dear mgorny thank you for comments. There is now a updated ebuild pushed into PR that tries to fix most of the problems detected in your review. There is a note and a question regarding the comments. First is about simultanous use of INSTALL_ROOT and DESTDIR and answer is yes - without modifying upstream we need both. Passing both INSTALL_ROOT and DESTDIR avoids me from creating a patch against upstream makefile so passing both here is smaller pain. The question is about very last comment "Finish it with /.. Trailing slash is undefined behavior." that I don't understand. Is it about dodoc directory or what? Installing docs to doc/doxygen.generated/html/.. would lead to .. nonfunctional documentation so obviously there is some details that I don't get here. Where shall I put .. in order to avoid undefined behaviour? -- |
If they indeed need both, that's fine. Just wanted to make sure you didn't leave one or the other by mistake. As for the trailing slash part, it's POSIX. When you pass |
PR 9449: made documentation directory more POSIX conformant. Closes: https://bugs.gentoo.org/661338 Package-Manager: Portage-2.3.40, Repoman-2.3.9
Pull request CI report Report generated at: 2018-08-17 17:42 UTC Issues already there before the PR (double-check them): |
Looks good, thanks! |
EAPI=7 for the new upstream version.
Patches of 0.12 removed because upstream has changes included.
Package-Manager: Portage-2.3.40, Repoman-2.3.9
Closes: https://bugs.gentoo.org/661338