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

["Yann E. MORIN" <yann dot morin dot 1998 at anciens dot enib dot fr>]

Arnaud, All,

On Tuesday 03 August 2010 06:02:56 Arnaud Lacombe wrote:
> this does not say anything about trust, the source of patches, nor
> their quality. Has it been made by the project developers ? if so, why
> has it not been integrated ? Has it been made by someone who had the
> slightest knowledge of how the patched software work ? or by a student
> in internship working overtime for an integrator ?

This is a valid point. Unfortunately, that requires a *huge* amount of
time to check all of this. I do not have such time available, not do I
have the possibility to do so.

So there are a couple of sources that I do trust (eg. the gentoo guys,
the buildroot guys, and so on), and when those people come with a fix
I hapilly use that, and I try to keep attribution either in the patch
itself, or at least in the commit message. But I'm human, and I can
forget.

> 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:
- gcc
- binutils
- glibc

Which are the three packages that require the most patches...

> Moreover, upstream package should be tagged enough for a 
> lambda user not to send  bug-report to the upstream for a patch broken
> in ct.

That's what crosstool-NG does, by inserting the 'pkgversion' to the
gcc build. Not all packages have this feature, and to my knowledge,
only gcc has it.

> Harvesting patches to harvest patches (as Yann seems to be fond 
> of)

No, I'm not fond of it. I even am against the practice from an
ideological point of view.

But lets be pragmatic for a minute. Almost no upstream package that
make a toolchain (gcc, binutils, glibc, uClibc...) can be used in
their pristine state, and still allow for a functional toolchain.

In some cases, the package will not even build, or worse, will generate
incorrect code. For example, gcc defaults to armv5t for ARM EABI, and
even if armv4t (which is capable of doing EABI) is selected, then gcc
will hapilly generate armv5t instructions, and won't tell you.

> should be the last thing to do. I'm sure he would have been super 
> happy if I had included 42 differents patches with the upgrade patch
> to gcc 4.5.1. He would certainly never have check they were all fully
> valid; he is not a gcc devs, neither am I. Finally, I could have
> rooted every gcc 4.5.1 build with these patches.

No, I'm no gcc dev. But I do a sanity check on patches. Yes, it is not
an in-depth analysis, the patch can screw things in an non-obvious
way. But what protects me from upstream doing nasty tricks, so
nasty that I can not trust the compiler (cf. "Reflections on trusting
trust", by Ken Thompson)? Obviously, I can investigate the gcc code
even less than I can inspect a patch on gcc...

I'm afraid we have to live with that.

> 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. If applying
the whole patchset gives 0.9.30.3, then lets remove 0.9.30.2, and replace
it with 0.9.30.3.

Why does crosstool-NG still not offer 0.9.30.3? Well, I was happy with
0.9.30.2, did not have a good reason to upgrade myself, and no one offered
the upgrading patch, or at least pointed to the fact that 0.9.30.3 was out
(I was aware it was, but *I* did not have no incentive to move forward).

> 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.

130: OK, that one is non-obvious. And I don't have time to invesstigate.

> 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.

> 150-LT-pthread_atfork-unhide.patch
>  -> fix perl build, ok

I'm replying here to your other comment:
> 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.

> 160-Make-use-of-macros-from-sys-asm.h-in-crt1.S.patch
>  -> fix mips+nptl boot, AFAIK, nptl is not in 0.9.30.2: trash

Yes. Patch to correct the current state?

> 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.

> 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.

> 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.

> 210-fix-make-install-host-utils.patch
>  -> seems to fix a build: ok
> 
> 220-fstatat-fix-up-behavior-on-32-64-bit-hosts.patch
>  -> behavioral change: trash

To me, this is a fix to the fstatat function.

> 230-getdents-Fix-mips64-build.patch
>  -> mips64 build fix: ok
> 
> 240-host-utils-depend-on-headers.patch
>  -> build fix: ok
> 
> 250-libc-Fix-typo-in-include-rpc.patch
>  -> fix typo: trash

Woo.. The typo is in the #define of the macro _GNU_SOURCE upon which
a non-negligible part of the code may rely.

> 260-libm-enable-log2f-and-exp2f.patch
>  -> functional change: trash

Well, why not.

> 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?...

> 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

> 290-blackfin-nommu-fork-stub.patch
>  -> provide fork(2) on blackfin: trash

Well, why not.

> From the patches of unknown origin, author or purpose, there is 2
> worth being pulled in. From buildroot set, there is barely 5 patches
> actually fixing build issue (out of 14), the rest is
> nitpicking/runtime change.

> The funniest for the end:  120-rm-whitespace.patch : this is pure
> whitespace change... I don't know if I should laught or cry...

Hmmm. I'll take this for a humorous sentence. See above for the explanations.

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.

If you really need a trusted toolchain, and manage to get paid to review
*all* of the code that gets used to build it, then all the better for you.
You're ewelcome to report back the issues you see, and come up with
patches (and hard explanations about them) to fix them.

And if you don't trust the patches bundled with crosstool-NG, then you have
the option to not use them at all. There is an option for that in the
menuconfig. You can even point crostool-NG to use an external patchset, and
use it in-lieu of the bundled one.

Well, I've been home for 1h30 now, and all I've done is this mail.
Now, on to some other, more interesting stuff.

Regards,
Yann E. MORIN.

PS. I've tried not to be offensive in this mail, but yours was a bit harsh.
YEM.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'



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