Bug 234750 - Review Request: avr-binutils - Cross Compiling GNU binutils targeted at avr
Review Request: avr-binutils - Cross Compiling GNU binutils targeted at avr
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Trond Danielsen
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-01 09:07 EDT by Hans de Goede
Modified: 2010-07-13 11:37 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-04-23 16:44:38 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
trond.danielsen: fedora‑review+
tcallawa: fedora‑cvs+


Attachments (Terms of Use)
Review report (9.38 KB, text/plain)
2007-04-01 10:10 EDT, Trond Danielsen
no flags Details
Complete review (9.37 KB, text/html)
2007-04-20 12:33 EDT, Trond Danielsen
no flags Details

  None (edit)
Description Hans de Goede 2007-04-01 09:07:04 EDT
Spec URL: http://people.atrpms.net/~hdegoede/avr-binutils.spec
SRPM URL: http://people.atrpms.net/~hdegoede/avr-binutils-2.17.50.0.12-1.fc7.src.rpm
Description:
This is a Cross Compiling version of GNU binutils, which can be used to
assemble and link binaries for the avr platform, instead of for the
native %{_arch} platform.

--

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.
Comment 1 Hans de Goede 2007-04-01 09:09:14 EDT
Note this package is identical to the arm-linux-binutils package except for the
%define target. For avr-binutils review see: bug 234749
Comment 2 Trond Danielsen 2007-04-01 10:10:37 EDT
Created attachment 151389 [details]
Review report
Comment 3 Trond Danielsen 2007-04-01 10:16:20 EDT
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 +.
Comment 4 Ralf Corsepius 2007-04-01 13:42:16 EDT
(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



Comment 5 Hans de Goede 2007-04-01 13:55:09 EDT
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.)
Comment 6 Ralf Corsepius 2007-04-01 14:02:00 EDT
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.
Comment 7 Ralf Corsepius 2007-04-01 14:07:31 EDT
(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.

Comment 8 Hans de Goede 2007-04-01 14:15:09 EDT
(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.
Comment 9 Ralf Corsepius 2007-04-01 14:19:01 EDT
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]



Comment 10 Trond Danielsen 2007-04-01 14:46:23 EDT
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.
Comment 11 Hans de Goede 2007-04-01 15:13:10 EDT
(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.
Comment 12 Hans de Goede 2007-04-01 15:56:56 EDT
okay, I've made a new version with the following changes:
* Sun Apr  1 2007 Hans de Goede <j.w.r.degoede@hhs.nl> 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
Comment 13 Trond Danielsen 2007-04-03 08:52:39 EDT
I tried removing flex and bison from BuildRequirements, and the package still
builds in mock.
Comment 14 Hans de Goede 2007-04-03 09:03:02 EDT
(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?
Comment 15 Ralf Corsepius 2007-04-03 09:57:21 EDT
(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"?
Comment 16 Hans de Goede 2007-04-03 10:20:04 EDT
(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?
Comment 17 Ralf Corsepius 2007-04-03 10:26:30 EDT
(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. 

Comment 18 Ralf Corsepius 2007-04-04 10:30:53 EDT
* 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)
Comment 19 Hans de Goede 2007-04-06 05:02:22 EDT
(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

Comment 20 Trond Danielsen 2007-04-06 05:23:07 EDT
(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.

Comment 21 Ralf Corsepius 2007-04-06 23:37:59 EDT
(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.
Comment 22 Ralf Corsepius 2007-04-07 01:24:39 EDT
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".
Comment 23 Hans de Goede 2007-04-07 03:11:51 EDT
(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
Comment 24 Trond Danielsen 2007-04-07 03:26:05 EDT
(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.
Comment 25 Lennart Kneppers 2007-04-20 07:45:33 EDT
Whats the status on this one?
Comment 26 Trond Danielsen 2007-04-20 12:33:05 EDT
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.
Comment 27 Trond Danielsen 2007-04-22 07:50:29 EDT
Looks good to me; approved by Trond Danielsen.
Comment 28 Hans de Goede 2007-04-22 08:28:16 EDT
New Package CVS Request
=======================
Package Name:      avr-binutils
Short Description: Cross Compiling GNU binutils targeted at avr
Owners:            j.w.r.degoede@hhs.nl
Branches:          FC-6 devel
InitialCC:         <empty>
Comment 29 Hans de Goede 2007-04-23 16:44:38 EDT
Trond, thanks for the review! Ralf, thanks for all the input!

Imported and build, closing.

I'll post avr-gcc for review soonish.
Comment 30 Thibault North 2010-07-12 15:31:09 EDT
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 !
Comment 31 Tom "spot" Callaway 2010-07-13 11:37:37 EDT
CVS done (by process-cvs-requests.py).

Note You need to log in before you can comment on or make changes to this bug.