Re: ct-ng -> git repos instead of single patches

[Arnaud Lacombe <lacombar at gmail dot com>]

Hi,

On Tue, Aug 3, 2010 at 12:59 PM, Yann E. MORIN
<yann.morin.1998@anciens.enib.fr> wrote:
>> I'd be temped to only allow in ct patches which have been submitted to
>> upstream, with link to the associated bug report and a distinguishable
>> author.
>
> I do not have my copyright assigned to the FSF, so I can not contribute
> to the FSF-managed projects, such as:
you don't need copyright assignment for trivial patch.

>> As a quick example of my point, most of the patch of uClibc 0.9.30.2
>> have been copied in a single shot from buildroot. Buildroot commit log
>> is "Everything on the 0_9_30 branch since the release (0.9.30.3 to
>> be)". So that why currently someone building a toolchain based on
>> uClibc 0.9.30.2 really has 0.9.30.3. I'd suspect that currently, Yann
>> does not provide uClibc 0.9.30.3 because "ct-ng" 0.9.30.2 is already
>> 0.9.30.3.
>
> There are known bugs in uClibc-0.9.30.2, let's fix those.
>
that's uclibc's people charge, not "ours".

>> Now, let's look at what patch are applied to uClibc 0.9.30.2:
>
> OK, lets investigate those...
>
>> 100-fix-gethostent_r-failure-retval.patch:
>> 110-arm_fix_alignment.patch:
>> 120-rm-whitespace.patch:
>> 130-gnu89-inline.patch:
>> ?-> from gentoo, no patch reason, not in upstream
>
> 100: one liner. Easy to understand. Plus, I followed the issue
> on the uClibc ML.
>
> 110: has a header stating what it does. Obvious.
>
> 120: Ah! the rm-whitespace issue! Did you even read what the patch does?
> The way the macros are used is incorrect. There shall be *no* space
> between the macros name and the openning parenthesis. The patch fixes
> that. Reading the patch makes this obvious.
>
then why is it not upstream ? btw, I cannot find this requirement in
the C99 spec...

>> 140-pack-netinet-structs.patch
>> ?-> patch from Joachim Nilsson, no more info about why it has not been
>> submitted upstream
>
> That one is non-obvious, but when you know how the kernel passes network
> data in structs to the userland, obviously those are packed.
>
> Eg. on x86, the following struct will not have holes in it, while on
> x86_64, it will:
> ?struct { long int a; long int b; } foo;
>
> Took me 2 minutes to realise the issue, and see the patch is probably
> right. Probably, as I looked only very fast. But the name of the patch
> is quite explanatory, and the knowing the above fact maeks the patch
> look OK.
>
then why is it not upstream ?

>> I'm re-thinking about this one, actually it's not ok as it fixes an
>> external package build failure, not the build of the toolchain
>> per-itself.
>
> The patch fixes a breakage in the perl build. But did it occur to you
> that it might be a bug *in uClibc* that induces that breakage?
> Note, I'm not sure, but to me the patch seems legit.
>
in uclibc, yes, not in ct-ng.

>> 170-Makefile.in-Make-install_dev-depend-on-install_runti.patch
>> ?-> "help" (ie. not "fix") // build, not critical: trash
>
> It is not critical, indeed.
> But the patch is signed-off-by a few of the people I _do_ trust, two
> of them being active devs of the project.
>
that no reason to include non-critical stuff.

>> 180-Unbreak-build-for-sparc-on-some-config-s.patch
>> ?-> unbreak sparc build: ok
>>
>> 190-avr32-add-varargs-handling-of-prctl-syscall.patch
>> ?-> patch for an atmel dev, but prctl(2) is hardly critical: trash
>
> It is not critical to _you_. But it fixes an actual bug, and some people
> may use it. A bug is a bug, and deserves being fixed.
>
then they should upgrade to uclibc 0.9.30.3.

>> 200-clean-up-O_CLOEXEC-handling.patch
>> ?-> looks functionnal change, no build failure reported: trash
>
> I like what the patch does, as it forces O-CLOEXEC in a functions that
> accesses the shadow passwd database. Seems sensible to me that children
> do not inherit that file descriptor.
>
this is not a question of you liking whatever or not...

>> 220-fstatat-fix-up-behavior-on-32-64-bit-hosts.patch
>> ?-> behavioral change: trash
>
> To me, this is a fix to the fstatat function.
>
then upgrade to 0.9.30.3

>> 260-libm-enable-log2f-and-exp2f.patch
>> ?-> functional change: trash
>
> Well, why not.
because if you want that fixed, upgrade to 0.9.30.3.

>> 270-malloc-fix-race-condition-and-other-bugs-in-the-no-m.patch
>> ?-> fix race condition: trash
>
> Why? This is a fix on a race condition, and you want to keep it?...
>
yes, it is a flaw in 0.9.30.2, fixed 0.9.30.3

>> 280-rpc-fix-typo-in-version-mismatch-msg.patch
>> ?-> typo: trash
>
> Typo in _actual code_. The code will mis-behave if you don't fix that.
> RPC_VERSMISMATCH = 6
> RPC_MISMATCH=0
>
yes, it is a flaw in 0.9.30.2, fixed 0.9.30.3

> Oh, by the way, I did all the above analysis in about 1 hour. On my free
> time, which would have been best spent in my real life.
>
What's your point ? You were free to ignore my mail and do something
better. How many time did I spent yesterday trying to get each patch
history ? Do I complain about it ?

> PS. I've tried not to be offensive in this mail, but yours was a bit harsh.
that was the point :)

Let's get my point of view, our job is to provide toolchains, not to
fix included software because it misbehaves functionally-wise or
because a patch add a fancy flower. That's the job of the upstream
project. If there is a fault, it should be reported to upstream and
fixed there, not in ct-ng only.

 - Arnaud

--
For unsubscribe information see http://sourceware.org/lists.html#faq