Bug 234750

Note this package is identical to the arm-linux-binutils package except for the
%define target. For avr-binutils review see: bug 234749

Created attachment 151389 [details]
Review report

This looks good to me. I've gone through the entire Review Guidelines, and
cannot find anything that should be a problem. I have tested building the
package in mock on both rawhide-i386 and fc6-x86_64, and there were no problems.

The complete review report is available in comment #2

If there are no other objections I will set the fedora-review flag to +.

(In reply to comment #3)
> This looks good to me. I've gone through the entire Review Guidelines, and
> cannot find anything that should be a problem. I have tested building the
> package in mock on both rawhide-i386 and fc6-x86_64, and there were no problems.
> 
> The complete review report is available in comment #2
> 
> If there are no other objections I will set the fedora-review flag to +.

VETO - Allow this package a couple of days for further checking.

MUSTFIX without  having checked details yet.

* --disable-nls
nls can't be enabled unless the version is identical to Fedora's.

* --target-prefix
Superfluous

I have no(In reply to comment #4)
> (In reply to comment #3)
> > This looks good to me. I've gone through the entire Review Guidelines, and
> > cannot find anything that should be a problem. I have tested building the
> > package in mock on both rawhide-i386 and fc6-x86_64, and there were no problems.
> > 
> > The complete review report is available in comment #2
> > 
> > If there are no other objections I will set the fedora-review flag to +.
> 
> VETO - Allow this package a couple of days for further checking.
> 
> MUSTFIX without  having checked details yet.
> 
> * --disable-nls
> nls can't be enabled unless the version is identical to Fedora's.
> 

Actually currently the version is identical to Fedora's (for Fedora 7), but can
you explain this a bit more, what is nls, and what do we loose by disabling it?

Also why must the version be identical to Fedora in order to be able to enable this?

> * --target-prefix
> Superfluous
> 

Nope, I thought so too, but %configure does something which makes this necessary
(probably passing -bindir).


Last remark to both you and Trond, what do you think about my initial question:

---

Notice to reviewers, I've removed the info / manpages and the PO files as these
conflict with the native binutils. I think that this means that this package
should have a "Requires: bintuils" to make sure the native binutils are always
installed, especially for the PO files, I haven't done this yet as I'm not sure,
but I think such a Requires should be added.

---

So should this require the native binutils for the PO files (which are btw an
other reason to try and keep the native and our version in sync.)

Further remarks:

* The upstream for binutils sources is
ftp://ftp.gnu.org/pub/gnu/binutils/
http://www.gnu.org/software/binutils/

You are packaging H.J.Lu's linux fork of the linux-binutils. IMO, you are badly
advised to package them for non-linux targets.


* BuildRequires: libtools
Superfluous.


* BuildRequires: flex and bison
Likely to be superfluous - Nominally, building binutils does NOT requires them,
except that there exist some broken versions of binutils which (due to bugs) do.
I haven't checked what applies to this version.


* This is not correct:
..
%{name} does not include any documentation (info / manpages) because these
would conflict with the native binutils. Please see the docs for the native
binutils, so instead of "man %{target}-as" use "man as". For additional docs
also see: %{_docdir}/binutils-%{version}
..

You can ship all section 1 man-pages, they do not conflict.

(In reply to comment #5)
> I have no(In reply to comment #4)
> > (In reply to comment #3)
>
> > * --disable-nls
> > nls can't be enabled unless the version is identical to Fedora's.
> > 
> 
> Actually currently the version is identical to Fedora's (for Fedora 7), 
> but can you explain this a bit more, what is nls, and what do we loose by
> disabling it?
This is a random match - IMO, actually a fault, because you are using the wrong
binutils-sources for this target.

> Also why must the version be identical to Fedora in order to be able to
> enable this?
At run-time gettext applies "sprintf-format string" lookups.

At the very moment the format strings inside of an application will mismatch
with that of the installed locale/ files (e.g. because format strings have been
added/removed), you'll be facing address violations / memory leaks.

These typically show as sporatic/hardly reproducable run-time errors.

(In reply to comment #6)
> Further remarks:
> 
> * The upstream for binutils sources is
> ftp://ftp.gnu.org/pub/gnu/binutils/
> http://www.gnu.org/software/binutils/
> 
> You are packaging H.J.Lu's linux fork of the linux-binutils. IMO, you are badly
> advised to package them for non-linux targets.
> 

Ok, this is what Fedora uses as a base so i started there, but I believe you,
so I will use the official 2.17 for the next iteration. I will also add the
advised --disable-nls configure flag. Side note, what is wise for arm-linux, the
Fedora version or the GNU one?

> * This is not correct:
> ..
> %{name} does not include any documentation (info / manpages) because these
> would conflict with the native binutils. Please see the docs for the native
> binutils, so instead of "man %{target}-as" use "man as". For additional docs
> also see: %{_docdir}/binutils-%{version}
> ..
> 
> You can ship all section 1 man-pages, they do not conflict.
> 

True, I will fix this.

Open questions:
1 If we use a different version as native, then some translated strings may be 
  changed, leading to them not being translated. I guess the differences are 
  small, so that we can live with this, but I would like to hear what others 
  think.
2 In order for translations to work at all (and to have the info pages) the
  native binutils must be installed, so I'm tending towarda adding
  "Requires: binutils" to the specfile, but again I would like to hear what 
  others think.

In reply to comment #5)

> > * --target-prefix
> > Superfluous
> > 
> 
> Nope, I thought so too, but %configure does something which makes 
> this necessary (probably passing -bindir).
ATM, I am short on time, so just a short note without having tried to check details:

You observation probably is the result of one of two bugs in rpm:
- redhat-rpm-config kills config.sub/config.guess and replaces them with ancient
versions. This is inappropriate for binutils, gcc etc. because they ship with
much newer versions, esp. for those targettting embedded systems, which often
ship customized versions.

- %configure overrides --host, --build ...
This is wrong for cross-compilation.

The work-around to both issues is not to use %configure, but to
directly use configure with all "--*dir" options explicitly passed directly

[adding jwilson, because this bug is one of those I repeatedly had complained about]

First of all: Sorry for my initial and to early report. Thanks to Ralf for
finding all the errors that I missed.

(In reply to comment #8)
> Open questions:
> 1 If we use a different version as native, then some translated strings may be 
>   changed, leading to them not being translated. I guess the differences are 
>   small, so that we can live with this, but I would like to hear what others 
>   think.

As Ralf mentioned, this might lead to sporadic errors at run-time, which is
something that should be avoided. The gain in the added translation does not
justify this potential problem.

(In reply to comment #10)
> First of all: Sorry for my initial and to early report. Thanks to Ralf for
> finding all the errors that I missed.
> 
> (In reply to comment #8)
> > Open questions:
> > 1 If we use a different version as native, then some translated strings may be 
> >   changed, leading to them not being translated. I guess the differences are 
> >   small, so that we can live with this, but I would like to hear what others 
> >   think.
> 
> As Ralf mentioned, this might lead to sporadic errors at run-time, which is
> something that should be avoided. The gain in the added translation does not
> justify this potential problem.

Ah I see now, /me stupid, --disable-nls disables the use of translations, yes
thats an excellent solution, thanks Ralf!

Working on a new version now.

okay, I've made a new version with the following changes:
* Sun Apr  1 2007 Hans de Goede <j.w.r.degoede> 2.17-1
- Revert to GNU 2.17 release as using GNU releases are better for non linux
  targets
- Add --disable-nls, to disable translations, so that we don't use the native
  PO files, as using the PO files of the (different version) native binutils,
  can lead to all kinda problems when translating formatstrings.
- Don't use %%configure but DIY, to avoid unwanted side effects of %%configure

Go get it here:
Spec URL: http://people.atrpms.net/~hdegoede/avr-binutils.spec
SRPM URL: http://people.atrpms.net/~hdegoede/avr-binutils-2.17-1.fc7.src.rpm

I tried removing flex and bison from BuildRequirements, and the package still
builds in mock.

(In reply to comment #13)
> I tried removing flex and bison from BuildRequirements, and the package still
> builds in mock.

Yes, I know, but I left them in deliberately, because as Ralf said, sometimes
having them present is needed to work around certain bugs. And the ./configure
script does check for them. Also they are not that bug. So all things concidered
I would rather leave them in.

I think that covers all mentioned issues, can this package be approved now then?

(In reply to comment #14)
> (In reply to comment #13)
> > I tried removing flex and bison from BuildRequirements, and the package still
> > builds in mock.
> 
> Yes, I know, but I left them in deliberately, because as Ralf said, sometimes
> having them present is needed to work around certain bugs.
I recommend to remove them from BuildRequires, because this inside of a
build-system assures building to use the pre-build sources and prevents an
arbitrary bison/byacc/flex to interfere. I'd even go a step further and to 
BuildConflicts: bison flex byacc

> And the ./configure
> script does check for them. Also they are not that bug. So all things
> concidered I would rather leave them in.
Well you know about esp. flex's "fame"?

(In reply to comment #15)
> (In reply to comment #14)
> > And the ./configure
> > script does check for them. Also they are not that bug. So all things
> > concidered I would rather leave them in.
> Well you know about esp. flex's "fame"?

No I don't but I trust your experience with cross compiler setups (mine is very
limited) so I'll take your word for it. Are there any other problems with the
second revision I posted other then that those BR's should be removed?

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)

> Are there any other problems with the
> second revision I posted other then that those BR's should be removed?
Sorry, but I haven't had any chance to look into it yet. 

* MUSTFIX:
Useless man-pages
/usr/share/man/man1/avr-dlltool.1.gz
/usr/share/man/man1/avr-nlmconv.1.gz
/usr/share/man/man1/avr-windres.1.gz

These are Win tools, not being useful nor built for embedded avr-targets.
binutils installing them is a bug in binutils-2.17.*

* RECOMMENDATIONS:
- You are exporting CFLAGS:
export CFLAGS="$RPM_OPT_FLAGS"
./configure ...

This doesn't do any harm in this particular case (binutils is an ordinary,
native-only package), but will be harmful when a package applies
cross-compilation (e.g. when building GCC).

I recommend to pass CFLAGS on the configure command-line instead, i.e.
CFLAGS="$RPM_OPT_FLAGS" ./configure ...

This hard-codes CFLAGS into generated files and avoids conflicts between
different CFLAGS between Makefiles and environment.

- I'd recommend to build this package inside of an avr-binutils-... directory
instead of "binutils-..." (apply %setup magic)

- I'd recommend to build binutils "VPATH style" instead of "in-source-tree
style". Though this is not required by building binutils, it much less
error-prone than in-source-tree-builts (and required when building GCC)


In my specs I do all these steps this way:
%prep
%setup -q -c -T -n %{name}-%{version}
%setup -q -D -T -n %{name}-%{version} -a0

%build
mkdir -p build
cd build
CFLAGS="$RPM_OPT_FLAGS" ../binutils-%{version}/configure ....
cd ..

Finally: As you already know, I dislike a toolchains using an architecture (avr)
as their target, because this doesn't make much sense and actually is wrong if
wanting to be pedantic. But I don't want to insist on this, in this particular
case, because the avr always had been "special" and probably doesn't have a long
enough history (Other targets with a longer history have learnt their leasons:
i386-elf, m68k-elf, m68k-coff, arm-eabi)

(In reply to comment #18)
> In my specs I do all these steps this way:
> %prep
> %setup -q -c -T -n %{name}-%{version}
> %setup -q -D -T -n %{name}-%{version} -a0
> 

Erm why not just:
%prep
%setup -q -c

That has exactly the same effect.

> Finally: As you already know, I dislike a toolchains using an architecture (avr)
> as their target

I know, but as long as almost the whole world does it like this I'm not planning
on changing this.

All other Must's and Should's fixed, new version here:
Spec URL: http://people.atrpms.net/~hdegoede/avr-binutils.spec
SRPM URL: http://people.atrpms.net/~hdegoede/avr-binutils-2.17-2.fc7.src.rpm

(In reply to comment #19)
> (In reply to comment #18)
> > Finally: As you already know, I dislike a toolchains using an architecture (avr)
> > as their target
> 
> I know, but as long as almost the whole world does it like this I'm not planning
> on changing this.

Even though I in principle agree with Ralf, I think it is to much trouble to try
to convince the rest of the world. As everybody else uses only avr as target, I
 think it justifies the exception.

(In reply to comment #19)
> (In reply to comment #18)
> > In my specs I do all these steps this way:
> > %prep
> > %setup -q -c -T -n %{name}-%{version}
> > %setup -q -D -T -n %{name}-%{version} -a0
> > 
> 
> Erm why not just:
> %prep
> %setup -q -c
> 
> That has exactly the same effect.
Yes, but only if a package has exactly one source-tarball.

When a package has several source-tarballs (like my gcc-specs), my construct is
more flexible (I generate the specs)

> > Finally: As you already know, I dislike a toolchains using an architecture (avr)
> > as their target
> 
> I know, but as long as almost the whole world does it like this I'm 
> not planning on changing this.
Well, ... who is this whole world you are referring to?

Any GNU toolchain developer will tell you that using an architecture as target
is not a clever decision.

MUSTFIX: 

Using mkdir is not safe against "rpmbuild --short-circuit":

# rpmbuild -bb avr-binutils.spec

# rpmbuild --short-circuit -bc avr-binutils.spec
Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.5345
...
+ mkdir build
mkdir: cannot create directory `build': File exists
error: Bad exit status from /var/tmp/rpm-tmp.5345 (%build)

Replace the "mkdir build" with "mkdir -p build".

(In reply to comment #22)
> MUSTFIX: 
> 
> Using mkdir is not safe against "rpmbuild --short-circuit":
> 
> Replace the "mkdir build" with "mkdir -p build".

Fixed, new version here:
Spec URL: http://people.atrpms.net/~hdegoede/avr-binutils.spec
SRPM URL: http://people.atrpms.net/~hdegoede/avr-binutils-2.17-3.fc7.src.rpm

(In reply to comment #21)
> (In reply to comment #19)
> > I know, but as long as almost the whole world does it like this I'm 
> > not planning on changing this.
> Well, ... who is this whole world you are referring to?

The whole world is everybody else who distributes compilers for the AVR; the
biggest one being WinAVR, but also every one *nix distribution that I know of.

> Any GNU toolchain developer will tell you that using an architecture as target
> is not a clever decision.

Silly suggestion: Could we compile avr-binutils with the correct target, and
then add additional symlinks with the names that most people would expect? That
way we gradually educate people while remaining backward compatible with
existing project.

Whats the status on this one?

Created attachment 153195 [details]
Complete review

As far as I can see this package is ok. Unless there are any additional
comments I will mark it as approved later today.

Looks good to me; approved by Trond Danielsen.

New Package CVS Request
=======================
Package Name:      avr-binutils
Short Description: Cross Compiling GNU binutils targeted at avr
Owners:            j.w.r.degoede
Branches:          FC-6 devel
InitialCC:         <empty>

Trond, thanks for the review! Ralf, thanks for all the input!

Imported and build, closing.

I'll post avr-gcc for review soonish.

Package Change Request
======================
Package Name: avr-binutils
New Branches: EL-6
Owners: tnorth trondd

We would like to have FEL-related packages available for EL-6. Thanks !

CVS done (by process-cvs-requests.py).