[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#773992: RFS: xmlrpc-c/1.33.15+svn20141223~2672-1 [ITA]



Hi Jörg,

On 30-12-14 21:17, Jörg Frings-Fürst wrote:
> first sorry for my late answer. I was i little bit ooo and then the
> 31C3.

No problem. Also sorry I didn't replied sooner.

> Am Montag, den 29.12.2014, 11:13 +0100 schrieb Paul Gevers:
>> On 26-12-14 20:48, Jörg Frings-Fürst wrote:
>> Ironically, upstream just (15 hours ago) seems to have released 1.33.15
>> as tar ball.
> 
> I have seen it too. But the download diretory is "Xmlrpc-c Super
> Stable/". I thinks its only a new Super Stable release. 

Sure, saw that. I guess you want to follow the stable release tree?

>>>   * New debian/patches/200-test_port.diff:
>>>     - Change port for testing from 8080 to 7890 (Closes: #722503).
>>
>> I am missing some background, either in the bug or in the patch file.
>> Why do you think hard coding 7890 is any better than 8080? And why do
>> you consider this "Forwarded: not-needed"? Michael suggested to try
>> multiple ports instead of relying on one port and I think upstream
>> should be interested in such a patch.
> 
> My first opinion was to change nothing. On debian the builds was running
> in a clean system without network. So there are no changes required.
> 
> On a build on a normal system port 8080 often be used by proxies or http
> servers. So I switched the port for the build tests to 7890.
> 
> I think that the using of multiple ports in this case the coast are to
> high.
> 
> In my opinion is the best way to set the port for testing at build time
> as configure variables (--testport=7890  or so). But this must be
> discussed with upstream. So have set "Forwarded: not-needed". 

Well, some of this information was/is very welcome in the patch header.
Maybe with a link where to the e-mail thread where you started the
discussion about it (so that in the far future we can check what
actually became of the request).

>>>   * New debian/patches/005-xmlrpc_example.diff:
>>>     - Backport from upstream release 1.34.0 (Closes: #524550).
>>
>> If you still want me to upload your current package, could you add a
>> source URL to the DEP5 header? The bts seems to say it should be here:
>> http://sourceforge.net/p/xmlrpc-c/code/2491 but with that commit I don't
>> see any content there. At least mention the proper revision in the
>> headers. Also, the patch seems to fix more than just the bug in the bts.
>> Please document what it is supposed to fix.
>>
> 
> I have rewriten the patch, so that only the wrong example is removed.

Ack.

>>>   * New missing debian/xmlrpc-c-config.man and debian/xmlrpc.man.
>>
>> You created these files with help2man. I prefer it when you do this at
>> build time, so that the man page stays up-to-date. I think you can tweak
>> the settings of help2man to not add the date and your name.
>>
> Yes the manfiles was basically made with help2man, but also massive
> edited. Therefore I don't build them at build time.

Then I really suggest you remove the first line of the man page file.
Did you send this manpage upstream then? Ideally you should be able to
drop this file again once upstream excepts it. Also I suggest to either
drop the statement about "may be used by others" or improve the wording
such that you actually really mention a common license name that applies
to your work on this man page.

>> Just wondering (haven't check yet), but you only add symbols files for
>> amd64 and i386. Is this working correctly with the other archs? Or are
>> they going to be more strict as a result?
>>
> My error. I have differences between amd64 and i386. So I have renamed
> the symbols file for libxmlrpc-c++8 to *.amd86 and *.i386.
> 
> I have renamed libxmlrpc-c++8.symbols.amd64 to libxmlrpc-c++8.symbols.

Wouldn't it be possible to merge the symbols files so that the common
part can still be used (haven't checked the content of the files myself
yet).

I think you don't need to add the version to the dpkg-gensymbols call,
and if you do, why strip the Debian part of the version? Doesn't
dh_makeshlibs call dpkg-gensymbols itself? So if you try to override
anything, shouldn't the dpkg-gensymbols calls be BEFORE the
dh_makeshlibs call? This doesn't look right to me. Have you seen
https://wiki.debian.org/UsingSymbolsFiles where it describes a way to
create a symbols file that contains as much history as possible?

>> Please separate your commits to git to ease review and understanding.
> 
> ok

Thanks for doing this a bit. However, your commit dbfaa7a7cc is horrible
to review though, because you mix upstream changes due to the new
release with your changes. I recommend to have the new upstream release
in a clean commit and then add your changes nicely documented afterwards.

>> I don't understand why you created a "new upstream release". As far as I
>> see it the only change is the version number and the distclean target in
>> the Makefile. No further changes in the files that you decided to keep
>> around.
>>
> 
> Are the upstream change from 1.33.14 to 1.33.15 not enough?

As I said, there weren't ANY. But as you now package a full new version,
this point is moot.

> A new version is uploaded to mentors.

Again, I look at the git version and assume it is the same.

Some new remarks:

It looks like you are mixing ideas in your d/rules file. You export
DEB_BUILD_MAINT_OPTIONS = hardening=+all, but at the same time you
declare all the *FLAGS manually (which you shouldn't). Also you don't
use or export the *FLAGS. I think you should check the last section of
dpkg-buildflags(1). (I could be wrong here). I think it would make
500-mk_gennnmtab.patch unnecessary.

Going to bed now, more review coming.

Paul


Attachment: signature.asc
Description: OpenPGP digital signature


Reply to: