[gentoo-dev] Re: [gentoo-commits] repo/gentoo:master commit in: net-libs/libssh2/

[Jeroen Roovers <jer@gentoo.org>]

OK, let's do a full review of your review.


On Mon, 01 Oct 2018 17:00:24 +0200
Michał Górny <mgorny@gentoo.org> wrote:

> On Mon, 2018-10-01 at 11:46 +0000, Jeroen Roovers wrote:
> > commit:     d866d4705e1e4a092579a31df2815e3407950a19
> > Author:     Jeroen Roovers <jer <AT> gentoo <DOT> org>
> > AuthorDate: Mon Oct  1 11:45:43 2018 +0000
> > Commit:     Jeroen Roovers <jer <AT> gentoo <DOT> org>
> > CommitDate: Mon Oct  1 11:46:10 2018 +0000
> > URL:
> > https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=d866d470
> > 
> > net-libs/libssh2: Add USE=mbedtls, switch to cmake for building
> > 
> > * Add support for net-libs/mbedtls
> > * Switch to cmake as the autotools build is even more broken
> > * Remove USE=static-libs as that inhibits building shared libs
> > * Use REQUIRED_USE to force choosing a crypto backend

You completely skipped over the improvements. In effect, you show
yourself to be completely unresponsive to what were considered positive
changes to the author of the work.

Then you begin to pick apart what you think is wrong. It's not obvious
why you are doing it this way, and with regard to practically all
of your earlier e-mails addressed to me, I can only assume malice.

> So your src_compile() is now broken for Ninja users, and src_test()
> is broken altogether.  How about cmake-multilib eclass?

ninja is a thing? Can you explain what it is? Can you show me how it's
broken, and how I reproduce the ninja problem, pro forma by filing a bug
report at https://bugs.gentoo.org/

> > +DESCRIPTION="Library implementing the SSH2 protocol"
> > +HOMEPAGE="https://www.libssh2.org"
> > +SRC_URI="https://www.${PN}.org/download/${P}.tar.gz"
> > +
> > +LICENSE="BSD"
> > +SLOT="0"
> > +KEYWORDS="~alpha ~amd64 ~arm ~arm64 ~hppa ~ia64 ~mips ~ppc ~ppc64
> > ~s390 ~sh ~sparc ~x86 ~amd64-fbsd ~x86-fbsd ~amd64-linux ~x86-linux
> > ~ppc-macos ~x64-macos ~x86-solaris" +IUSE="gcrypt libressl mbedtls
> > test zlib"  
> 
> IUSE=test is not used at all.

So that's a minor problem we could fix even without a revision bump.
You could have pointed it out in that missing bug report.

> > +REQUIRED_USE="
> > +	?? ( gcrypt libressl mbedtls )
> > +"
> > +
> > +RDEPEND="
> > +	!libressl?
> > ( >=dev-libs/openssl-1.0.1h-r2:0=[${MULTILIB_USEDEP}] )
> > +	gcrypt?
> > ( >=dev-libs/libgcrypt-1.5.3:0[${MULTILIB_USEDEP}] )
> > +	libressl? ( dev-libs/libressl:0=[${MULTILIB_USEDEP}] )
> > +	mbedtls? ( net-libs/mbedtls )  
> 
> By the way, MULTILIB_USEDEP certainly missing here.

This wants another explanation. How is this different from the ebuild
that went before, -r1? You don't even explain what you think the
problem is. You're not trying to be nice. I have to make an effort
to turn your comment into intelligible English.

MULTILIB_USEDEP *is* missing here? By here you presumably mean
net-libs/mbedtls needs it because that's the line your comment follows?
Right?

So you could have said:

That should be
mbedtls? ( net-libs/mbedtls[MULTILIB_USEDEP] )

> > +	zlib? ( >=sys-libs/zlib-1.2.8-r1[${MULTILIB_USEDEP}] )
> > +"
> > +DEPEND="${RDEPEND}"
> > +
> > +PATCHES=(
> > +	"${FILESDIR}"/${PN}-1.8.0-libgcrypt-prefix.patch  
> 
> You sure this autoconf patch is needed for CMake build?

It's harmless. See above. But you also know that it is not needed, but
you're not kindly pointing that out, you're just taking any opportunity
to make a snide comment.

> > +	"${FILESDIR}"/${PN}-1.8.0-mansyntax_sh.patch
> > +	"${FILESDIR}"/${PN}-1.8.0-openssl11-memleak.patch
> > +	"${FILESDIR}"/${PN}-1.8.0-openssl11.patch
> > +)
> > +
> > +multilib_src_configure() {
> > +	local crypto_backend=OpenSSL
> > +	if use gcrypt; then
> > +		crypto_backend=Libgcrypt
> > +	elif use mbedtls; then
> > +		crypto_backend=mbedTLS
> > +	fi
> > +
> > +	local mycmakeargs=(
> > +		-DBUILD_SHARED_LIBS=ON
> > +		-DCRYPTO_BACKEND=${crypto_backend}
> > +		-DENABLE_ZLIB_COMPRESSION=$(usex zlib)
> > +	)
> > +	cmake-utils_src_configure
> > +}
> > +
> > +multilib_src_install_all() {
> > +	einstalldocs
> > +	find "${D}" -name '*.la' -delete || die  
> 
> They must have hacked that CMake hard to get it to generate .la files.

So again it's harmless? But you're in snide mode now.

> > +	mv "${D}"/usr/share/doc/${PN}/*
> > "${D}"/usr/share/doc/${PF}/ || die
> > +	rm -r "${D}"/usr/share/doc/${PN}/ || die
> > +}
> > 
> > diff --git a/net-libs/libssh2/metadata.xml
> > b/net-libs/libssh2/metadata.xml index e9e734ab02f..2149f2ed0c6
> > 100644 --- a/net-libs/libssh2/metadata.xml
> > +++ b/net-libs/libssh2/metadata.xml
> > @@ -10,5 +10,7 @@
> >  </maintainer>
> >  <use>
> >  	<flag name="gcrypt">Use <pkg>dev-libs/libgcrypt</pkg>
> > instead of <pkg>dev-libs/openssl</pkg></flag>
> > +	<flag name="libressl">Use <pkg>dev-libs/libressl</pkg>
> > instead of <pkg>dev-libs/openssl</pkg></flag>
> > +	<flag name="mbedtls">Use <pkg>net-libs/mbedtls</pkg>
> > instead of <pkg>dev-libs/openssl</pkg></flag> </use>
> >  </pkgmetadata>

You're quoting the rest because you agree? Or what? OK, let's go back
to the beginning:

> > net-libs/libssh2: Add USE=mbedtls, switch to cmake for building
> > 
> > * Add support for net-libs/mbedtls
> > * Switch to cmake as the autotools build is even more broken
> > * Remove USE=static-libs as that inhibits building shared libs
> > * Use REQUIRED_USE to force choosing a crypto backend

I spent a lot of time trying to figure out why and how the autotools
build system was broken and altogether too much time figuring out why it
couldn't be fixed and that upstream isn't very much interested in why
it's broken and how it can be fixed. When the autotools route failed
hard, I recovered at least some of the work by adding mbedtls support
and switching to cmake, which is probably the way forward in so far as
upstream intentions can be read. The only thing you came up with in
response is a couple of minor issues mostly wrapped in snide
oneliner remarks. In the same time you could have fixed the issues
yourself and congratulated yourself and the rest of the team on getting
ahead with libssh2 despite impossible odds.

But of course that is not what happened. This is what happened (in the
natural order of events, reversed from how git likes it):

commit 4cb94ede1a2a8d2b57e866d1fb2b909b857806d0
Author: Michał Górny <mgorny@gentoo.org>
Date:   Thu Aug 24 09:01:24 2017 +0200

    net-libs/libssh2: Add myself as backup maint (dep of libgit2)

[A stabilisation effort]
[Pacho Ramos fixing a security bug]
[Another stabilisation effort]

commit 8698aae507b6cd43aa3accec8cd5af4fe1ecdea5
Author: Michał Górny <mgorny@gentoo.org>
Date:   Thu Sep 6 22:46:57 2018 +0200

    net-libs/libssh2: Clean old up

commit d866d4705e1e4a092579a31df2815e3407950a19
Author: Jeroen Roovers <jer@gentoo.org>
Date:   Mon Oct 1 13:45:43 2018 +0200

    net-libs/libssh2: Add USE=mbedtls, switch to cmake for building

    * Add support for net-libs/mbedtls
    * Switch to cmake as the autotools build is even more broken
    * Remove USE=static-libs as that inhibits building shared libs
    * Use REQUIRED_USE to force choosing a crypto backend

    Package-Manager: Portage-2.3.50, Repoman-2.3.11
    Signed-off-by: Jeroen Roovers <jer@gentoo.org>

Someone suggested in an e-mail that "he is just annoyed that you broke
an ebuild that he has spent some time maintaining", but `git shortlog
-- .` would tell you quite a different story. What I think is happening
here is that you think I am "touching your stuff". You have to nitpick
at it instead of just fixing it together and then sending me an update
about the extra work or by pointing out the problems in a more humane
way and leaving me to fix them.

The way you're "working" on the problem means I am not really inclined
to fix any problems for you. So drop the attitude. Nobody likes it.



     jer