net-im/psi update #5219

Ri0n · 2017-07-26T08:28:31Z

Updated 9999 version.
Bumped release to 1.2

Removed dependency on psimedia from new ebuilds. So psimedia could be removed without any harm. psimedia will be fixed someday to work either with new gstreamer of with qt internal multimedia framework.

More ebuilds will be submitted later. Also some fixes will be added to these ones. particularly related to internationalization, which is incomplete in these ebuilds.

gentoo-repo-qa-bot · 2017-07-26T08:31:02Z

Pull Request assignment

Areas affected: ebuilds, profiles
Packages affected: net-im/psi

net-im/psi: @gentoo/proxy-maint (maintainer needed)

a17r · 2017-07-26T09:43:26Z

net-im/psi/psi-1.2.ebuild

+	enchant? ( spell )
+	hunspell? ( spell )
+	webengine? ( webkit )
+	^^ ( qt4 qt5 )


Please avoid such constructs. It is an unnecessary burden to resolve, when qt4 can be simply done as !qt5.

However: Qt4 is going away, so it would be much better if you just dropped the option altogether and make it a qt5-only build.

a17r · 2017-07-26T09:46:03Z

net-im/psi/psi-1.2.ebuild

+		dbus? ( dev-qt/qtdbus:4 )
+		app-crypt/qca:2[qt4]
+		whiteboarding? ( dev-qt/qtsvg:4 )
+		webkit? ( dev-qt/qtwebkit:4 )


Qt4Webkit is in the process of removal, new ebuild submissions with it as a dependency will not be accepted. See also: https://bugs.gentoo.org/show_bug.cgi?id=620684

a17r · 2017-07-26T09:54:10Z

net-im/psi/psi-1.2.ebuild

+	)
+	qt5? (
+		dev-qt/qtgui:5
+		dev-qt/qtxml:5


This and other lines: Please keep dependencies sorted, it makes future maintenance much better. Optional dependencies below fixed, and also sorted.

a17r · 2017-07-26T17:14:29Z

net-im/psi/psi-1.2.ebuild

+	aspell? ( spell )
+	enchant? ( spell )
+	hunspell? ( spell )
+	webengine? ( webkit )


This is an unfortunate REQUIRED_USE:

The way webengine and webkit flags are currently used among most packages is preferring one over the other.

The way it is used in the ebuild, it seems you could instead do ?? ( webengine webkit ) and only --enable-webkit when one of them is selected.

Ri0n · 2017-07-27T07:08:35Z

I honestly don't understand what qa-bot wants from me :-)

a17r · 2017-07-27T07:40:55Z

The remaining two CI issues are not caused by your PR.

Wrt previous failure, if you have a USE flag that due to dependencies does not work on all arches, you can mask that flag in the according profiles/arch/<arch>/package.use.mask files. However, not everything provided by the build system strictly needs to be exposed by USE flags if it is not that important.

a17r

thanks for your work - please address the remaining nitpicks, sqash ebuild changes and have a look at https://wiki.gentoo.org/wiki/Gentoo_git_workflow#Commit_policy to adjust the commit message. repoman commit, besides doing basic QA and dependency checks, already prepares half of a proper commit message for you.

a17r · 2017-07-27T07:45:30Z

net-im/psi/psi-1.2.ebuild

+	emake INSTALL_ROOT="${D}" install
+
+	# this way the docs will be installed in the standard gentoo dir
+	rm -f "${ED}"/usr/share/${MY_PN}/{COPYING,README}


I'd rather do without -f and add the missing || die

a17r · 2017-07-27T07:50:43Z

net-im/psi/psi-1.2.ebuild

+	aspell? ( app-text/aspell )
+	dbus? ( dev-qt/qtdbus:5 )
+	enchant? ( >=app-text/enchant-1.3.0 )
+	hunspell? ( app-text/hunspell )


this probably needs := when it links to a versioned soname

a17r · 2017-07-27T07:56:59Z

net-im/psi/psi-1.2.ebuild

+		--qtdir="$(qt5_get_bindir)/.."
+	)
+
+	use dbus || CONF+=("--disable-qdbus")


this and other lines - any reason why you can't use $(use_enable) and move those into the CONF block above?

a17r · 2017-07-27T07:59:31Z

net-im/psi/psi-1.2.ebuild

+PLOCALES="be bg ca cs de en eo es et fa fi fr he hu it ja kk mk nl pl pt pt_BR ru sk sl sr@latin sv sw uk ur_PK vi zh_CN zh_TW"
+PLOCALE_BACKUP="en"
+
+SRC_URI="mirror://sourceforge/${PN}/${P}.tar.xz


Please move this below the HOMEPAGE line.

a17r · 2017-07-27T08:04:08Z

net-im/psi/psi-1.2.ebuild

+RESTRICT="test"
+
+pkg_setup() {
+	MY_PN=psi


There is no reason to do this in pkg_setup (usually we declare those on top, after EAPI), but MY_PN is the same as PN anyway unless I'm missing something?

a17r · 2017-07-27T08:05:49Z

net-im/psi/psi-9999.ebuild

-		ewarn "that fail too."
+		ewarn "You're about to build patched version of Psi called Psi+."
+		ewarn "It has new nice features not yet included to Psi."
+		ewarn "Take a look at homepage for more info: http://psi-plus.com/"


Just a suggestion: You can use ${HOMEPAGE} there and it will always be up to date with the ebuild.

That's Psi+ home. HOMEPAGE is just Psi. But thanks. maybe I will use it somewhere else :-)

thanks - my eyes ;)

a17r · 2017-07-27T08:09:46Z

net-im/psi/psi-9999.ebuild

-		use powersave && epatch "${WORKDIR}/psi-plus/patches/dev/psi-reduce-power-consumption.patch"
+		PATCHES_DIR="${WORKDIR}/psi-plus/patches"
+		EPATCH_SOURCE="${PATCHES_DIR}" EPATCH_SUFFIX="diff" EPATCH_FORCE="yes" epatch
+		use sql && epatch "${PATCHES_DIR}/dev/psi-new-history.patch"


In EAPI 6 we don't use epatch anymore. Please convert to eapply.

a17r · 2017-07-27T08:16:47Z

net-im/psi/psi-1.2.ebuild

@@ -6,62 +6,49 @@ EAPI=6
 PLOCALES="be bg ca cs de en eo es et fa fi fr he hu it ja kk mk nl pl pt pt_BR ru sk sl sr@latin sv sw uk ur_PK vi zh_CN zh_TW"
 PLOCALE_BACKUP="en"

-SRC_URI="mirror://sourceforge/${PN}/${P}.tar.xz"
+SRC_URI="mirror://sourceforge/${PN}/${P}.tar.xz
+	https://github.com/psi-im/psi-l10n/archive/1.2.tar.gz -> psi-l10n-${PV}.tar.gz"

 inherit eutils l10n multilib qmake-utils


multilib.eclass seems unused

a17r · 2017-07-27T08:17:06Z

net-im/psi/psi-9999.ebuild

-		EPATCH_EXCLUDE="${MY_EPATCH_EXCLUDE} " \
-		EPATCH_SOURCE="${WORKDIR}/psi-plus/patches/" EPATCH_SUFFIX="diff" EPATCH_FORCE="yes" epatch
+		if use iconsets; then
+			cp -a "${WORKDIR}/resources/iconsets" "${S}" || die	"failed to copy additional iconsets"


that looks like a tab after die

a17r · 2017-07-27T08:17:54Z

net-im/psi/psi-1.2.ebuild

+SRC_URI="mirror://sourceforge/${PN}/${P}.tar.xz
+	https://github.com/psi-im/psi-l10n/archive/1.2.tar.gz -> psi-l10n-${PV}.tar.gz"
+
+inherit eutils l10n multilib qmake-utils


I don't see multilib.eclass in use here - and after converting to eapply you should also be able to drop eutils.eclass - please check.

a17r · 2017-07-27T18:10:57Z

net-im/psi/metadata.xml

@@ -1,16 +1,21 @@
 <?xml version='1.0' encoding='UTF-8'?>
 <!DOCTYPE pkgmetadata SYSTEM "http://www.gentoo.org/dtd/metadata.dtd">
 <pkgmetadata>
-	<!-- maintainer-needed -->
-	<longdescription>Psi is a very good jabber client that uses QT</longdescription>
+	<maintainer type="person">


In addition you also need to add proxy-maint team, like so: https://wiki.gentoo.org/wiki/Project:Proxy_Maintainers/User_Guide#Proxied_maintainer_in_metadata.xml

it's there now

a17r · 2017-07-27T20:11:12Z

Bug https://bugs.gentoo.org/show_bug.cgi?id=511462 suggests to add RESTRICT="iconsets? ( bindist )" because of unclear licensing, this would be a good opportunity to fix as well if still applicable.

Ri0n · 2017-07-27T21:04:34Z

forgot about tab. fixed now. but had to squash all together

SoapGentoo · 2017-07-30T10:35:52Z

net-im/psi/psi-1.2.ebuild

+
+src_configure() {
+	CONF=(
+		--libdir="${EPREFIX}"/usr/$(get_libdir)


not necessary in EAPI 6

ah true. after converting to econf I forgot to remove this.

SoapGentoo · 2017-07-30T10:36:00Z

net-im/psi/psi-1.2.ebuild

+	CONF=(
+		--libdir="${EPREFIX}"/usr/$(get_libdir)
+		--no-separate-debug-info
+		--prefix="${EPREFIX}"/usr


also not necessary

SoapGentoo · 2017-07-30T10:37:00Z

net-im/psi/psi-1.2.ebuild

+	if use doc; then
+		cd doc
+		make api_public || die "make api_public failed"
+	fi


turn this whole if block into use doc && emake -C doc api_public

SoapGentoo · 2017-07-30T10:37:57Z

net-im/psi/psi-1.2.ebuild

+	newdoc certs/README README.certs
+	dodoc README
+
+	use doc && dohtml -r doc/api


dohtml is deprecated. Either use docinto html + dodoc -r doc/api/. or HTML_DOCS + einstalldocs

SoapGentoo · 2017-07-30T10:38:08Z

net-im/psi/psi-1.2.ebuild

+
+	# install translations
+	local mylrelease="$(qt5_get_bindir)"/lrelease
+	cd "${WORKDIR}/psi-l10n-${PV}"


missing || die

SoapGentoo · 2017-07-30T10:39:20Z

net-im/psi/psi-9999.ebuild

+		EGIT_REPO_URI="${PSI_PLUS_LANGS_URI}"
+	else
+		EGIT_REPO_URI="${PSI_LANGS_URI}"
+	fi


tip (you don't have to do this): EGIT_REPO_URI=$(usex extras "${PSI_PLUS_LANGS_URI}" "${PSI_LANGS_URI}")

SoapGentoo · 2017-07-30T10:39:46Z

net-im/psi/psi-9999.ebuild

+		features="$(use webkit && echo '--webkit') $(use webengine && echo '--webengine') $(use sql && echo '--sql')"
+		NIGHTLY_VER=$("${vergen}" ./ $features)
+		elog "Prepared version: ${NIGHTLY_VER}"
+		echo "${NIGHTLY_VER}" > version


missing || die

a17r · 2017-07-31T17:59:51Z

umm something went wrong there - could you please remove the merge commit and rebase against master? then I think we will be good to proceed here.

gentoo-repo-qa-bot · 2017-07-31T19:57:19Z

😞 The QA check for this pull request has found the following issues:

Issues inherited from Gentoo (may be modified by PR):
https://qa-reports.gentoo.org/output/gentoo-ci/ad65a773f/output.html#sys-auth/nss-pam-ldapd

a17r · 2017-07-31T20:25:29Z

Thanks for your work - ebuilds look in pretty good shape now safe for some really minor cosmetic nitpicks I am going to solve in a separate commit.

Closes: gentoo#5219

a17r · 2017-07-31T21:24:02Z

@Ri0n I've both altered your commit messages to be git workflow compliant, mention related bugs, and did the mentioned cosmetic fixes and then some more - please have a look.

Closes: #5219

gentoo-repo-qa-bot added maintainer-needed There is at least one affected package with no maintainer. Review it if you can. assigned PR successfully assigned to the package maintainer(s). labels Jul 26, 2017

a17r suggested changes Jul 26, 2017

View reviewed changes

a17r suggested changes Jul 27, 2017

View reviewed changes

SoapGentoo suggested changes Jul 30, 2017

View reviewed changes

Ri0n added 2 commits August 1, 2017 00:29

net-im/psi version bump

e3a2efd

Don't mask net-im/psi-1.2

41be375

a17r approved these changes Jul 31, 2017

View reviewed changes

a17r pushed a commit to a17r/gentoo that referenced this pull request Jul 31, 2017

net-im/psi: 1.2 version bump, switch to Qt5

f5b5a53

Closes: gentoo#5219

a17r pushed a commit to a17r/gentoo that referenced this pull request Jul 31, 2017

profiles: Restrict net-im/psi package.mask to <1.0

a8b5f17

Closes: gentoo#5219

SoapGentoo approved these changes Jul 31, 2017

View reviewed changes

gentoo-bot closed this in e32db49 Jul 31, 2017

gentoo-bot pushed a commit that referenced this pull request Jul 31, 2017

profiles: Restrict net-im/psi package.mask to <1.0

2504754

Closes: #5219

net-im/psi update #5219

net-im/psi update #5219

Conversation

Ri0n commented Jul 26, 2017

gentoo-repo-qa-bot commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ri0n commented Jul 27, 2017

a17r commented Jul 27, 2017 • edited

a17r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a17r Jul 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a17r commented Jul 27, 2017

Ri0n commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a17r commented Jul 31, 2017 • edited

gentoo-repo-qa-bot commented Jul 31, 2017

a17r commented Jul 31, 2017

a17r commented Jul 31, 2017

a17r commented Jul 27, 2017 •

edited

a17r Jul 27, 2017 •

edited

a17r commented Jul 31, 2017 •

edited