Gentoo Archives: gentoo-dev

From: Jeroen Roovers <jer@g.o>
To: "Michał Górny" <mgorny@g.o>
Cc: qa <qa@g.o>, comrel@g.o, gentoo-dev@l.g.o
Subject: [gentoo-dev] Re: [gentoo-commits] repo/gentoo:master commit in: net-libs/libssh2/
Date: Mon, 01 Oct 2018 21:13:15
Message-Id: 20181001231303.0386d024@wim.jer
1 OK, let's do a full review of your review.
2
3
4 On Mon, 01 Oct 2018 17:00:24 +0200
5 Michał Górny <mgorny@g.o> wrote:
6
7 > On Mon, 2018-10-01 at 11:46 +0000, Jeroen Roovers wrote:
8 > > commit: d866d4705e1e4a092579a31df2815e3407950a19
9 > > Author: Jeroen Roovers <jer <AT> gentoo <DOT> org>
10 > > AuthorDate: Mon Oct 1 11:45:43 2018 +0000
11 > > Commit: Jeroen Roovers <jer <AT> gentoo <DOT> org>
12 > > CommitDate: Mon Oct 1 11:46:10 2018 +0000
13 > > URL:
14 > > https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=d866d470
15 > >
16 > > net-libs/libssh2: Add USE=mbedtls, switch to cmake for building
17 > >
18 > > * Add support for net-libs/mbedtls
19 > > * Switch to cmake as the autotools build is even more broken
20 > > * Remove USE=static-libs as that inhibits building shared libs
21 > > * Use REQUIRED_USE to force choosing a crypto backend
22
23 You completely skipped over the improvements. In effect, you show
24 yourself to be completely unresponsive to what were considered positive
25 changes to the author of the work.
26
27 Then you begin to pick apart what you think is wrong. It's not obvious
28 why you are doing it this way, and with regard to practically all
29 of your earlier e-mails addressed to me, I can only assume malice.
30
31 > So your src_compile() is now broken for Ninja users, and src_test()
32 > is broken altogether. How about cmake-multilib eclass?
33
34 ninja is a thing? Can you explain what it is? Can you show me how it's
35 broken, and how I reproduce the ninja problem, pro forma by filing a bug
36 report at https://bugs.gentoo.org/
37
38 > > +DESCRIPTION="Library implementing the SSH2 protocol"
39 > > +HOMEPAGE="https://www.libssh2.org"
40 > > +SRC_URI="https://www.${PN}.org/download/${P}.tar.gz"
41 > > +
42 > > +LICENSE="BSD"
43 > > +SLOT="0"
44 > > +KEYWORDS="~alpha ~amd64 ~arm ~arm64 ~hppa ~ia64 ~mips ~ppc ~ppc64
45 > > ~s390 ~sh ~sparc ~x86 ~amd64-fbsd ~x86-fbsd ~amd64-linux ~x86-linux
46 > > ~ppc-macos ~x64-macos ~x86-solaris" +IUSE="gcrypt libressl mbedtls
47 > > test zlib"
48 >
49 > IUSE=test is not used at all.
50
51 So that's a minor problem we could fix even without a revision bump.
52 You could have pointed it out in that missing bug report.
53
54 > > +REQUIRED_USE="
55 > > + ?? ( gcrypt libressl mbedtls )
56 > > +"
57 > > +
58 > > +RDEPEND="
59 > > + !libressl?
60 > > ( >=dev-libs/openssl-1.0.1h-r2:0=[${MULTILIB_USEDEP}] )
61 > > + gcrypt?
62 > > ( >=dev-libs/libgcrypt-1.5.3:0[${MULTILIB_USEDEP}] )
63 > > + libressl? ( dev-libs/libressl:0=[${MULTILIB_USEDEP}] )
64 > > + mbedtls? ( net-libs/mbedtls )
65 >
66 > By the way, MULTILIB_USEDEP certainly missing here.
67
68 This wants another explanation. How is this different from the ebuild
69 that went before, -r1? You don't even explain what you think the
70 problem is. You're not trying to be nice. I have to make an effort
71 to turn your comment into intelligible English.
72
73 MULTILIB_USEDEP *is* missing here? By here you presumably mean
74 net-libs/mbedtls needs it because that's the line your comment follows?
75 Right?
76
77 So you could have said:
78
79 That should be
80 mbedtls? ( net-libs/mbedtls[MULTILIB_USEDEP] )
81
82 > > + zlib? ( >=sys-libs/zlib-1.2.8-r1[${MULTILIB_USEDEP}] )
83 > > +"
84 > > +DEPEND="${RDEPEND}"
85 > > +
86 > > +PATCHES=(
87 > > + "${FILESDIR}"/${PN}-1.8.0-libgcrypt-prefix.patch
88 >
89 > You sure this autoconf patch is needed for CMake build?
90
91 It's harmless. See above. But you also know that it is not needed, but
92 you're not kindly pointing that out, you're just taking any opportunity
93 to make a snide comment.
94
95 > > + "${FILESDIR}"/${PN}-1.8.0-mansyntax_sh.patch
96 > > + "${FILESDIR}"/${PN}-1.8.0-openssl11-memleak.patch
97 > > + "${FILESDIR}"/${PN}-1.8.0-openssl11.patch
98 > > +)
99 > > +
100 > > +multilib_src_configure() {
101 > > + local crypto_backend=OpenSSL
102 > > + if use gcrypt; then
103 > > + crypto_backend=Libgcrypt
104 > > + elif use mbedtls; then
105 > > + crypto_backend=mbedTLS
106 > > + fi
107 > > +
108 > > + local mycmakeargs=(
109 > > + -DBUILD_SHARED_LIBS=ON
110 > > + -DCRYPTO_BACKEND=${crypto_backend}
111 > > + -DENABLE_ZLIB_COMPRESSION=$(usex zlib)
112 > > + )
113 > > + cmake-utils_src_configure
114 > > +}
115 > > +
116 > > +multilib_src_install_all() {
117 > > + einstalldocs
118 > > + find "${D}" -name '*.la' -delete || die
119 >
120 > They must have hacked that CMake hard to get it to generate .la files.
121
122 So again it's harmless? But you're in snide mode now.
123
124 > > + mv "${D}"/usr/share/doc/${PN}/*
125 > > "${D}"/usr/share/doc/${PF}/ || die
126 > > + rm -r "${D}"/usr/share/doc/${PN}/ || die
127 > > +}
128 > >
129 > > diff --git a/net-libs/libssh2/metadata.xml
130 > > b/net-libs/libssh2/metadata.xml index e9e734ab02f..2149f2ed0c6
131 > > 100644 --- a/net-libs/libssh2/metadata.xml
132 > > +++ b/net-libs/libssh2/metadata.xml
133 > > @@ -10,5 +10,7 @@
134 > > </maintainer>
135 > > <use>
136 > > <flag name="gcrypt">Use <pkg>dev-libs/libgcrypt</pkg>
137 > > instead of <pkg>dev-libs/openssl</pkg></flag>
138 > > + <flag name="libressl">Use <pkg>dev-libs/libressl</pkg>
139 > > instead of <pkg>dev-libs/openssl</pkg></flag>
140 > > + <flag name="mbedtls">Use <pkg>net-libs/mbedtls</pkg>
141 > > instead of <pkg>dev-libs/openssl</pkg></flag> </use>
142 > > </pkgmetadata>
143
144 You're quoting the rest because you agree? Or what? OK, let's go back
145 to the beginning:
146
147 > > net-libs/libssh2: Add USE=mbedtls, switch to cmake for building
148 > >
149 > > * Add support for net-libs/mbedtls
150 > > * Switch to cmake as the autotools build is even more broken
151 > > * Remove USE=static-libs as that inhibits building shared libs
152 > > * Use REQUIRED_USE to force choosing a crypto backend
153
154 I spent a lot of time trying to figure out why and how the autotools
155 build system was broken and altogether too much time figuring out why it
156 couldn't be fixed and that upstream isn't very much interested in why
157 it's broken and how it can be fixed. When the autotools route failed
158 hard, I recovered at least some of the work by adding mbedtls support
159 and switching to cmake, which is probably the way forward in so far as
160 upstream intentions can be read. The only thing you came up with in
161 response is a couple of minor issues mostly wrapped in snide
162 oneliner remarks. In the same time you could have fixed the issues
163 yourself and congratulated yourself and the rest of the team on getting
164 ahead with libssh2 despite impossible odds.
165
166 But of course that is not what happened. This is what happened (in the
167 natural order of events, reversed from how git likes it):
168
169 commit 4cb94ede1a2a8d2b57e866d1fb2b909b857806d0
170 Author: Michał Górny <mgorny@g.o>
171 Date: Thu Aug 24 09:01:24 2017 +0200
172
173 net-libs/libssh2: Add myself as backup maint (dep of libgit2)
174
175 [A stabilisation effort]
176 [Pacho Ramos fixing a security bug]
177 [Another stabilisation effort]
178
179 commit 8698aae507b6cd43aa3accec8cd5af4fe1ecdea5
180 Author: Michał Górny <mgorny@g.o>
181 Date: Thu Sep 6 22:46:57 2018 +0200
182
183 net-libs/libssh2: Clean old up
184
185 commit d866d4705e1e4a092579a31df2815e3407950a19
186 Author: Jeroen Roovers <jer@g.o>
187 Date: Mon Oct 1 13:45:43 2018 +0200
188
189 net-libs/libssh2: Add USE=mbedtls, switch to cmake for building
190
191 * Add support for net-libs/mbedtls
192 * Switch to cmake as the autotools build is even more broken
193 * Remove USE=static-libs as that inhibits building shared libs
194 * Use REQUIRED_USE to force choosing a crypto backend
195
196 Package-Manager: Portage-2.3.50, Repoman-2.3.11
197 Signed-off-by: Jeroen Roovers <jer@g.o>
198
199 Someone suggested in an e-mail that "he is just annoyed that you broke
200 an ebuild that he has spent some time maintaining", but `git shortlog
201 -- .` would tell you quite a different story. What I think is happening
202 here is that you think I am "touching your stuff". You have to nitpick
203 at it instead of just fixing it together and then sending me an update
204 about the extra work or by pointing out the problems in a more humane
205 way and leaving me to fix them.
206
207 The way you're "working" on the problem means I am not really inclined
208 to fix any problems for you. So drop the attitude. Nobody likes it.
209
210
211
212 jer

Replies