Bug 226795

Summary: Review Request: sdcc - Small Device C Compiler
Product: [Fedora] Fedora Reporter: Trond Danielsen <trond.danielsen>
Component: Package ReviewAssignee: Hans de Goede <hdegoede>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: alain.portal, cz172638, gwync, hdegoede, jcapik, jeff
Target Milestone: ---Keywords: Reopened
Target Release: ---Flags: hdegoede: fedora-review+
gwync: fedora-cvs+
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-01-31 12:06:23 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
Remove bogus compiler flags
none
Misc spec changes none

Description Trond Danielsen 2007-02-01 17:19:29 UTC
Spec URL: ftp://open-gnss.org/pub/fedora/sdcc.spec
SRPM URL: ftp://open-gnss.org/pub/fedora/sdcc-2.6.0-1.fc6.src.rpm
Description: 
SDCC is a C compiler for 8051 class and similar microcontrollers.
The package includes the compiler, assemblers and linkers, a device
simulator and a core library. The processors supported (to a varying
degree) include the 8051, ds390, z80, hc08, and PIC.

Comment 1 Trond Danielsen 2007-02-01 17:20:28 UTC
rpmlint output:
[/var/lib/mock/fedora-6-x86_64-core/result] rpmlint sdcc-2.6.0-1.fc6.x86_64.rpm 
[/var/lib/mock/fedora-6-x86_64-core/result] rpmlint
sdcc-devel-2.6.0-1.fc6.x86_64.rpm 
W: sdcc-devel no-documentation
E: sdcc-devel script-without-shebang /usr/share/sdcc/lib/src/time.c
E: sdcc-devel zero-length /usr/share/sdcc/lib/src/pic/libdev/p16c620a.c
E: sdcc-devel script-without-shebang /usr/share/sdcc/lib/src/gets.c
E: sdcc-devel zero-length /usr/share/sdcc/lib/large/dummy.lib
E: sdcc-devel zero-length /usr/share/sdcc/lib/small/dummy.rel
E: sdcc-devel script-without-shebang /usr/share/sdcc/lib/src/ds400/tinibios.c
E: sdcc-devel script-without-shebang /usr/share/sdcc/lib/src/ds390/rtc390.c
E: sdcc-devel zero-length /usr/share/sdcc/lib/small-stack-auto/dummy.lib
E: sdcc-devel zero-length /usr/share/sdcc/lib/small-stack-auto/dummy.rel
E: sdcc-devel zero-length /usr/share/sdcc/lib/medium/dummy.rel
E: sdcc-devel script-without-shebang /usr/share/sdcc/lib/src/ds390/tinibios.c
E: sdcc-devel zero-length /usr/share/sdcc/lib/large/dummy.rel
E: sdcc-devel zero-length /usr/share/sdcc/lib/small/dummy.lib
E: sdcc-devel script-without-shebang /usr/share/sdcc/lib/src/_itoa.c
E: sdcc-devel script-without-shebang /usr/share/sdcc/lib/src/ser_ir_cts_rts.c
E: sdcc-devel zero-length /usr/share/sdcc/lib/medium/dummy.lib
E: sdcc-devel script-without-shebang /usr/share/sdcc/lib/src/_ltoa.c



Comment 2 Brett L. Trotter 2007-02-01 23:51:10 UTC
This would be very, very great to have in the extras or even base tree.

Comment 3 Jeffrey C. Ollie 2007-02-02 05:24:39 UTC
Well, blow me down... I've been working the past few days on a SDCC
package as well but hadn't quite gotten my package to the point of
being able to post a review request.  A few items based upon what I've
seen so far (but not a full review yet):

1. The "script-without-shebang" errors can be fixed with this:

    find . -type f -name \*.c | xargs chmod a-x

2. The zero length file errors can be ignored IMHO, it looks like
   those files are required for proper functioning, even though they
   are empty.

3. What about adding "libgc-devel" to the BR and --enable-libgc to the
   %configure line?  From what I saw in the documentation this will
   help improve memory usage.  I don't really know much about SDCC so
   I don't know if that would mean other tradeoffs.

4. What about adding "latex2html" to the BR and --enable-doc to the
   %configure file?  This would allow the documentation to be included
   in the package.

5. The devel package doesn't own "%{_datadir}/sdcc".

6. Why remove the emacs files? Why not move them to
   "%{_datadir}/emacs/site-lisp/". That would make them easier to use
   if someone wanted to.

7. The main package isn't very useful without the -devel subpackage.
   Even though it will cause rpmlint to complain, what about having
   the main package require the -devel subpackage, or even eliminate
   the -devel subpackage and have just one package (even though that
   will cause rpmlint to complain even louder).

8. Is this being packaged in preparation for packaging GNU Radio?
   That's why I was packaging SDCC, but my GNU Radio package is in
   even less polished shape than my SDCC package.



Comment 4 Ralf Corsepius 2007-02-02 05:50:25 UTC
Many of the apps in %{_bindir} carry very generic names:

# rpm -qlp sdcc-2.6.0-1.i386.rpm 
/usr/bin/as-gbz80
/usr/bin/as-hc08
/usr/bin/as-z80
/usr/bin/aslink
/usr/bin/asx8051
/usr/bin/link-gbz80
/usr/bin/link-hc08
/usr/bin/link-z80
/usr/bin/makebin
/usr/bin/packihx
/usr/bin/s51
/usr/bin/savr
/usr/bin/sdcc
/usr/bin/sdcclib
/usr/bin/sdcdb
/usr/bin/sdcpp
/usr/bin/shc08
/usr/bin/sz80

IMO, all but the sdc-prefixed ones are way too generic. Does this package
support --program-prefix?

Also, I am pretty sure the debug infos are broken.


Comment 5 Jeffrey C. Ollie 2007-02-02 06:09:34 UTC
(In reply to comment #4)
>
> IMO, all but the sdc-prefixed ones are way too generic. Does this package
> support --program-prefix?

While I see your point here, I can see using --program-prefix potentially
causing problems with dependent packages...

> Also, I am pretty sure the debug infos are broken.

I noticed that as well.  Could it be the -ggdb flag that it adds to the CFLAGS
that causes the problems?

Comment 6 Ralf Corsepius 2007-02-02 07:03:08 UTC
(In reply to comment #5)
> (In reply to comment #4)

> > Also, I am pretty sure the debug infos are broken.
> 
> I noticed that as well.  Could it be the -ggdb flag that it adds to the CFLAGS
> that causes the problems?
I haven't checked details yet, but this and other CFLAGS-weirdnesses (Some piece
use -g3, some other use -gstab<something>, sorry I just deleted the log) are
likely candidates.

Another candidate is rpmbuild/redhat-rpm-config itself. It doesn't properly
distinguish between native and foreign object files. Working around this had
forced me to apply pretty nasty tricks in my cross-compiler rpms.


Comment 7 Trond Danielsen 2007-02-02 09:34:40 UTC
(In reply to comment #3)
> Well, blow me down... I've been working the past few days on a SDCC
> package as well but hadn't quite gotten my package to the point of
> being able to post a review request.  A few items based upon what I've
> seen so far (but not a full review yet):
> 
> 1. The "script-without-shebang" errors can be fixed with this:
> 
>     find . -type f -name \*.c | xargs chmod a-x

FIXED

> 
> 2. The zero length file errors can be ignored IMHO, it looks like
>    those files are required for proper functioning, even though they
>    are empty.

Is there a typo in there? Should the zero length files be kept or not?

> 
> 3. What about adding "libgc-devel" to the BR and --enable-libgc to the
>    %configure line?  From what I saw in the documentation this will
>    help improve memory usage.  I don't really know much about SDCC so
>    I don't know if that would mean other tradeoffs.

FIXED.

I have added the neccessary Requires and BuildRequires, and the package build
just fine with --enable-libgc.

> 
> 4. What about adding "latex2html" to the BR and --enable-doc to the
>    %configure file?  This would allow the documentation to be included
>    in the package.

FIXED

For some reason, sdcc requires lyx to build the documentation, and lyx depends
on latex2html.

> 
> 5. The devel package doesn't own "%{_datadir}/sdcc".

Devel package removed, see 7.

> 
> 6. Why remove the emacs files? Why not move them to
>    "%{_datadir}/emacs/site-lisp/". That would make them easier to use
>    if someone wanted to.

FIXED

> 
> 7. The main package isn't very useful without the -devel subpackage.
>    Even though it will cause rpmlint to complain, what about having
>    the main package require the -devel subpackage, or even eliminate
>    the -devel subpackage and have just one package (even though that
>    will cause rpmlint to complain even louder).

I thought about that too, but rpmlint complained, so I created the devel
package. But not I have removed it again, because it makes more sense to keep
everything in one package.

> 
> 8. Is this being packaged in preparation for packaging GNU Radio?
>    That's why I was packaging SDCC, but my GNU Radio package is in
>    even less polished shape than my SDCC package.

It is! There are many things still missing, for instance AVR compiler and
linker. I have stared creating a spec file for avr-binutils, but I did not have
time to finish it. If you want to discuss this with me, you can contact me
either on #gnuradio on irc.freenode.net or on discuss-gnuradio.


Comment 8 Trond Danielsen 2007-02-02 09:36:19 UTC
(In reply to comment #5)
> (In reply to comment #4)
> >
> > IMO, all but the sdc-prefixed ones are way too generic. Does this package
> > support --program-prefix?
> 
> While I see your point here, I can see using --program-prefix potentially
> causing problems with dependent packages...

I tried building it with --program-prefix=sdcc, but it failed. I will have to
look into this a bit further, if this is a requirement.




Comment 9 Trond Danielsen 2007-02-02 09:41:01 UTC
(In reply to comment #5)
> (In reply to comment #4)
> >
> > IMO, all but the sdc-prefixed ones are way too generic. Does this package
> > support --program-prefix?
> 
> While I see your point here, I can see using --program-prefix potentially
> causing problems with dependent packages...
> 
Would it be better to install everything under /usr/sdcc-i386, as suggested for
cross compilers? http://fedoraproject.org/wiki/PackagingDrafts/CrossCompilers

Comment 10 Trond Danielsen 2007-02-02 09:55:50 UTC
(In reply to comment #7)
> (In reply to comment #3)
> > 4. What about adding "latex2html" to the BR and --enable-doc to the
> >    %configure file?  This would allow the documentation to be included
> >    in the package.
> 
> FIXED
> 
> For some reason, sdcc requires lyx to build the documentation, and lyx depends
> on latex2html.
> 
Correction: Lyx does _not_ depend on latex2html... BuildRequires added.

Comment 11 Ralf Corsepius 2007-02-02 10:12:36 UTC
(In reply to comment #9)
> (In reply to comment #5)
> > (In reply to comment #4)
> > >
> > > IMO, all but the sdc-prefixed ones are way too generic. Does this package
> > > support --program-prefix?
> > 
> > While I see your point here, I can see using --program-prefix potentially
> > causing problems with dependent packages...
> > 
> Would it be better to install everything under /usr/sdcc-i386, as suggested for
> cross compilers? http://fedoraproject.org/wiki/PackagingDrafts/CrossCompilers
No. This would conflict with GCC's conventions.

But what would work is to install the apps somewhere outside of %{_bindir},
e.g
/usr/lib/sdcc/bin
or
/usr/libexec/sdcc
and to install symlinks or wrapper scripts named "sdcc-<app>" into %{_bindir}

Other packages wanting to use the "non-prefixed" versions then could apply
PATH=/usr/lib/scdc/bin:$PATH (or similar) to access the unprefixed versions.



Comment 12 Trond Danielsen 2007-02-02 11:17:57 UTC
(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > >
> > > > IMO, all but the sdc-prefixed ones are way too generic. Does this package
> > > > support --program-prefix?
> > > 
> > > While I see your point here, I can see using --program-prefix potentially
> > > causing problems with dependent packages...
> > > 
> > Would it be better to install everything under /usr/sdcc-i386, as suggested for
> > cross compilers? http://fedoraproject.org/wiki/PackagingDrafts/CrossCompilers
> No. This would conflict with GCC's conventions.
> 
> But what would work is to install the apps somewhere outside of %{_bindir},
> e.g
> /usr/lib/sdcc/bin
> or
> /usr/libexec/sdcc
> and to install symlinks or wrapper scripts named "sdcc-<app>" into %{_bindir}
> 
> Other packages wanting to use the "non-prefixed" versions then could apply
> PATH=/usr/lib/scdc/bin:$PATH (or similar) to access the unprefixed versions.
> 
FIXED. Binaries are now installed into /usr/libexec/sdcc, and symlinks are
created in /usr/bin.

Updated spec and srpm files are available at ftp://open-gnss.org/pub/fedora


Comment 13 Ralf Corsepius 2007-02-05 10:15:02 UTC
1. Please increment the release number each time you push a new package for
review and post the corresponding URLs here. Not doing so makes it unnecessarily
hard for reviewers to track your activities.

2. Package doesn't honor RPM_OPTFLAGS
configure.in and ucsim/configure.in play tricks with debugger flags.



Comment 14 Trond Danielsen 2007-02-05 11:19:15 UTC
(In reply to comment #13)
> 1. Please increment the release number each time you push a new package for
> review and post the corresponding URLs here. Not doing so makes it unnecessarily
> hard for reviewers to track your activities.
> 
Sorry, I did not think about that. I have updated the spec file with the correct
release number, and uploaded it and the srpm file here:
ftp://open-gnss.org/pub/fedora/sdcc

> 2. Package doesn't honor RPM_OPTFLAGS
> configure.in and ucsim/configure.in play tricks with debugger flags.
> 
Hmm, I am not sure how to solved this. I found the function in configure.in that
modifies $CFLAGS, but should I modify configure and run autoconf first, or
modify configure directly?


Comment 15 Miao ZhiCheng 2007-02-12 06:53:51 UTC
== Not an official review as I'm not yet sponsored ==
Mock build for FC6 i386 is sucessfull

* MUST Items
 - rpmlint
  supressed rpmlint errors:
W: sdcc dangling-relative-symlink /usr/bin/sdcc-sdcc sdcc
W: sdcc devel-file-in-non-devel-package /usr/share/sdcc/lib/src/pic/libsdcc/uint
2fs.c
(...hundreds of files alike this)
 - The package must meet the  Packaging Guidelines.
  remove the explicit lib require of gc
 - A package must own all directories that it creates. 
  I think you must own %{_datadir}/sdcc/ %{_libexecdir}/sdcc/ instead of the
files in it.
SHOULD Items:
 - A copy of licence in doc

Comment 16 Hans de Goede 2007-02-24 11:14:23 UTC
Hi all,

Some remarks:
-I agree that having a seperate -devel package for the header files is bogus
-But I think that a -src subpackages containing the libc sources would be a good
 idea, as normally these aren't needed for sdcc to function, or am I missing
 something here?

As also said on the mailinglist I like the current spec, in general it looks
good. So I would like to review this and thus actually get it into extras.

However since the special nature of this and all the talk about other
corss-compilers, I would like to suggest having 2 reviewers. What I have in mind
is that I do a formal review and that Ralf looks over my shoulder, then when I
approve this package, Ralf reviews it too (which should be a no-op) when Ralf
then approves it too he sets fedora-review to + and the CVS procedure can be
started. Does this sound like a plan?


Comment 17 Trond Danielsen 2007-02-25 23:23:37 UTC
The symlinks have now been fixed. 

The rpmlint devel-file-in-non-devel-package warnings are still there, but as
mentioned in #16 and other comments, it does not make sense to put them in a
-devel file. However, I may put them in a separate file named sdcc-libc or
something like that. I do not know wheter the sdcc package is usable without the
libc package; I would have to check with the sdcc devs.

Updated files are available from ftp://open-gnss.org/pub/fedora/sdcc/

Comment 18 Ralf Corsepius 2007-02-26 07:40:14 UTC
Created attachment 148782 [details]
Remove bogus compiler flags

This patch is supposed to resolve the "package doesn't acknowledge
RPM_OPT_FLAGS" issues mentioned in comment #6

Comment 19 Ralf Corsepius 2007-02-26 08:21:34 UTC
Created attachment 148783 [details]
Misc spec changes

This patch addresses various issues with the spec:

- Add sdcc-2.6.0-configure.diff.
- Pass Q= to make to make building verbose.
- Add __os_install_post post-hacks to prevent brp-strip from processing
  foreign binaries.

The last item on this list is a brutal hack to work around limitations/bugs in
RH's brp-* scripts. I am using similar hacks for my cross-toolchain rpms for a
very long time.

Comment 20 Hans de Goede 2007-02-26 09:49:10 UTC
Ralf,

Good work and many thanks. Do you think that it would be possible to fix the brp
scripts in a generic way, maybe by using file on the bins and then parsing the
output? Then we could submit a patch to the brp-scripts so that the ugly hacks
can eventually be removed.

I know getting patches into rpm is HARD, but I've been speaking with Max Spevack
and Spot in person about this for about an hour yesterday at Fosdem (it was
great) and I have good hope after our meeting for getting patches into core in
general. Basicly the trick is if a core maintainer isn't responsive
(disagreements are an entire other issue) and you've tried several times then
its ok to send Spot a personal mail with bugzilla URL and explanation and he
will do his best to get things moving (and having talked to him in person I
believe he will truely do his best and that this should work).

Trond can you integrate's Ralf's work and then post a new SRPM for review then
I'll start a full review.


Comment 21 Trond Danielsen 2007-02-26 10:08:02 UTC
Patch from #18 applied and spec file from #19 rebuilt. Updated SRPM and spec
file at ftp://open-gnss.org/pub/fedora/sdcc/

Comment 22 Hans de Goede 2007-02-26 15:20:38 UTC
No full review yet (no time right now) but I've been taking a closer look and
the -debuginfo package is still empty. Adding "STRIP=true" to the make install
arguments fixes this. (or STRIP=touch, anything but STRIP=strip, which is
otherwise harmless).

No need to post a new package for this, but I just wanted to write this down
before I forget :)



Comment 23 Ralf Corsepius 2007-02-26 16:06:44 UTC
(In reply to comment #22)
> No full review yet (no time right now) but I've been taking a closer look and
> the -debuginfo package is still empty. Adding "STRIP=true" to the make install
> arguments fixes this. (or STRIP=touch, anything but STRIP=strip, which is
> otherwise harmless).

Why not disabling it at configuration time?

%configure .... STRIP=: 

Comment 24 Hans de Goede 2007-02-26 16:42:39 UTC
Yes, that will work too.


Comment 25 Hans de Goede 2007-02-26 21:01:49 UTC
MUST:
=====
0 rpmlint output is:
W: sdcc rpm-buildroot-usage %prep sed -e 's,find $RPM_BUILD_ROOT,find $RPM_BUIL
W: sdcc devel-file-in-non-devel-package /usr/share/sdcc/lib/src/pic/libsdcc/uin
W: sdcc devel-file-in-non-devel-package /usr/share/sdcc/lib/src/pic16/libc/ctyp
W: sdcc devel-file-in-non-devel-package /usr/share/sdcc/lib/src/pic/libm/floorf
W: sdcc devel-file-in-non-devel-package /usr/share/sdcc/lib/src/pic16/libc/stdl
<many many more like these>
* Package and spec file named appropriately
0 Packaged according to packaging guidelines
* License ok
* spec file is legible and in Am. English.
* Source matches upstream
* Compiles and builds on devel x86_64
* BR: ok
* No locales
* No shared libraries
* Not relocatable
0 Package owns / or requires all dirs
* No duplicate files & Permissions ok
* %clean & macro usage OK
* Contains code only
* %doc does not affect runtime, and isn't large enough to warrent a sub package
* no -devel package needed, no libs / .la files.
* no .desktop file required

MUST fix:
=========
* put all the files under /usr/share/sdcc/lib/src and the .asm files under
  /usr/share/sdcc/lib/* in a seprate -src subpackage. AFAIK these files are only
  needed when one wants to look at the innerworkings of the C-library and are 
  not needed for normal development, thus they shouldn't be part of the base
  package.
* Remove the "Requires:       gc" from the specfile, gc is a lib and an
  automatic dependency on the needed .so file will be generated.
* We all agree a -devel package is bogus so remove the devel subpackage instead
  of just commenting it
* sdcc's make install installs the docs under /usr/share/sdcc/doc, they
  should be installed under /usr/share/doc/sdcc-%{version} using %doc
  Tip: after the "make install" do:
  mv $RPM_BUILD_ROOT%{_datadir}/%{name}/doc installed-docs
  and then to %files add "%doc installed-docs/*"
* You must own the sdcc dirs the package create, under %files don't write:
  %{_libexecdir}/sdcc/*
  %{_datadir}/sdcc/*
  But write:
  %{_libexecdir}/%{name}
  %{_datadir}/%{name}
  Then the package will also own the %{_datadir}/sdcc and %{_libexecdir}/sdcc
  dirs
* You must also own %{_datadir}/emacs as that is not a standard dir, easiest way
  todo this is to just write %{_datadir}/emacs under %files instead of
  %{_datadir}/emacs/site-lisp/*
  


Comment 26 Trond Danielsen 2007-02-27 10:49:21 UTC
(In reply to comment #25)
> MUST fix:
> =========
> * put all the files under /usr/share/sdcc/lib/src and the .asm files under
>   /usr/share/sdcc/lib/* in a seprate -src subpackage. AFAIK these files are only
>   needed when one wants to look at the innerworkings of the C-library and are 
>   not needed for normal development, thus they shouldn't be part of the base
>   package.

FIXED. 

> * Remove the "Requires:       gc" from the specfile, gc is a lib and an
>   automatic dependency on the needed .so file will be generated.

FIXED.

> * We all agree a -devel package is bogus so remove the devel subpackage instead
>   of just commenting it
> * sdcc's make install installs the docs under /usr/share/sdcc/doc, they
>   should be installed under /usr/share/doc/sdcc-%{version} using %doc
>   Tip: after the "make install" do:
>   mv $RPM_BUILD_ROOT%{_datadir}/%{name}/doc installed-docs
>   and then to %files add "%doc installed-docs/*"

FIXED.

> * You must own the sdcc dirs the package create, under %files don't write:
>   %{_libexecdir}/sdcc/*
>   %{_datadir}/sdcc/*
>   But write:
>   %{_libexecdir}/%{name}
>   %{_datadir}/%{name}
>   Then the package will also own the %{_datadir}/sdcc and %{_libexecdir}/sdcc
>   dirs

FIXED.

> * You must also own %{_datadir}/emacs as that is not a standard dir, easiest way
>   todo this is to just write %{_datadir}/emacs under %files instead of
>   %{_datadir}/emacs/site-lisp/*
>   

FIXED.

The Source URL has also been updated according to the guidelines in the wiki.
STRIP=: has also been added to %configure as suggested in #23. The debuginfo
package is still created though...

Updated spec and srpm files are here: ftp://open-gnss.org/pub/fedora/sdcc/


Comment 27 Hans de Goede 2007-02-27 21:06:26 UTC
Looks good,

A few last issues:
1) The changelog entry (and your last comment) about "Disable creation of 
   debuginfo package" is plain wrong. sdcc contains native binaries (the 
   compiler, linker et all). for which we want a debuginfo package, thus the
   debuginfo is a good thing. The problem was that it was an empty package.
   The changelog should read something like: "Disable stripping of binaries,
   so that we get a proper debuginfo package"
2) Remove the empty %doc from the "%files src"
3) The descripion of the -src subpackage is a bit vague, try explaning that
   these are the actual sources of the c-library for the devices and that these
   sources are meant for reference of how the c-library works.

Also I see that you need a sponsor, that is not a problem I can sponsor you, but
before doing that I would like todo one more package review with you, so can you
submit another package for review and post the bugzilla id here, then I'll reviw
it and assuming that goes well then sponsor you.

Ralf, do you agree with the modifications I've requested for the package? And
what do you think of the sdcc-src subpackage? Maybe sdcc-sources or
sdcc-libc-sources is better?


Comment 28 Ralf Corsepius 2007-02-28 02:46:55 UTC
(In reply to comment #27)
> Looks good,
> 
> A few last issues:
> 1) The changelog entry (and your last comment) about "Disable creation of 
>    debuginfo package" is plain wrong. sdcc contains native binaries (the 
>    compiler, linker et all). for which we want a debuginfo package, thus the
>    debuginfo is a good thing. The problem was that it was an empty package.
>    The changelog should read something like: "Disable stripping of binaries,
>    so that we get a proper debuginfo package"
Right, the changelog is wrong.

> 2) Remove the empty %doc from the "%files src"

> 3) The descripion of the -src subpackage is a bit vague, try explaning that
>    these are the actual sources of the c-library for the devices and that these
>    sources are meant for reference of how the c-library works.
> 
> Also I see that you need a sponsor, that is not a problem I can sponsor 
> you, but before doing that I would like todo one more package review with 
> you, so can you submit another package for review and post the bugzilla 
> id here, then I'll reviw it and assuming that goes well then sponsor you.
I don't feel able to sponsor anybody, because the ACL issues disable me from
being able to fulfil the tasks I consider to a sponsor's obligations :(

> Ralf, do you agree with the modifications I've requested for the package? And
> what do you think of the sdcc-src subpackage?
Well, I'd not have requested a 'src' package, because I don't see any use for
the sources anyway, ... but this is an issue upstream should take care about.

In same boat, is this package shipping the a target's library's *.o's in
parallel to libraries (*.lib, *.a). Normally this doesn't make any sense, 
... but this is an issue upstream should take care about.

> Maybe sdcc-sources or
> sdcc-libc-sources is better?
Hmm, I'm not sure. sdcc-libc-sources sounds like the most "self-explanatory"
package name to me, but this is a matter of personal preference.

Technically, I see directory ownership issues between *-src and the main package
(IMO, *-src must require the main package).

Finally, I don't think the "BR: byacc" is right. It probably should be "bison".
AFAIS, the toplevel configure seems to be wanting to enforce bison, but seems to
fail on this.

Comment 29 Hans de Goede 2007-02-28 09:18:43 UTC
(In reply to comment #28)
> (In reply to comment #27)
> > Also I see that you need a sponsor, that is not a problem I can sponsor 
> > you, but before doing that I would like todo one more package review with 
> > you, so can you submit another package for review and post the bugzilla 
> > id here, then I'll reviw it and assuming that goes well then sponsor you.
> I don't feel able to sponsor anybody, because the ACL issues disable me from
> being able to fulfil the tasks I consider to a sponsor's obligations :(
> 

Hmm, good point. I'll ask trond to open up the ACL for atleast the both of us
when its imported. Also I must say I've never actually needed write access to a
package of someone I sponsored so I'm okay with sponsering him even with the ACL's.

> In same boat, is this package shipping the a target's library's *.o's in
> parallel to libraries (*.lib, *.a). Normally this doesn't make any sense, 
> ... but this is an issue upstream should take care about.
> 

There are no .a files only .o or .rel files and .lib files, these .lib files are
not archives but linker scripts pointing to the .o / .rel files so that is fine.

> > Maybe sdcc-sources or
> > sdcc-libc-sources is better?
> Hmm, I'm not sure. sdcc-libc-sources sounds like the most "self-explanatory"
> package name to me, but this is a matter of personal preference.
> 

I like that the best too, Trond can you change the package name for the sources
to sdcc-libc-sources

> Technically, I see directory ownership issues between *-src and the main package
> (IMO, *-src must require the main package).
> 

Agreed, Trond can you add this Requires

> Finally, I don't think the "BR: byacc" is right. It probably should be "bison".
> AFAIS, the toplevel configure seems to be wanting to enforce bison, but seems to
> fail on this.

Well it seems to except both, bot to prefere bison, so indeed that BR
(BuildRequires) should be changed to bison.


Comment 30 Trond Danielsen 2007-02-28 11:26:28 UTC
(In reply to comment #27)
> Also I see that you need a sponsor, that is not a problem I can sponsor you, but
> before doing that I would like todo one more package review with you, so can you
> submit another package for review and post the bugzilla id here, then I'll reviw
> it and assuming that goes well then sponsor you.

I submitted another package:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=230324

and updated sdcc according to the comments; snippet from changelog:
* Wed Feb 28 2007 Trond Danielsen <trond.danielsen> - 2.6.0-6
- Renamed source code package to libc-sources.
- Change BuildRequire from byacc to bison.
- Added "Require: sdcc" to libc-sources package.
- Empty %doc entry removed.
- Updated description of libc-sources package.

Updated files at same location as previously: ftp://open-gnss.org/pub/fedora/sdcc



Comment 31 Hans de Goede 2007-02-28 11:46:12 UTC
(In reply to comment #30)
> 
> and updated sdcc according to the comments; snippet from changelog:
> * Wed Feb 28 2007 Trond Danielsen <trond.danielsen> - 2.6.0-6
> - Renamed source code package to libc-sources.
> - Change BuildRequire from byacc to bison.
> - Added "Require: sdcc" to libc-sources package.

I'm afraid one more iteration is needed as that should be:
Requires:       sdcc = %{version}-%{release}

As we always want this package and the base package to be in sync.

Otherwise things look good so with that last change this package can be
approved. I'll take a look at avrdude next.


Comment 32 Trond Danielsen 2007-02-28 12:01:15 UTC
(In reply to comment #31)
> I'm afraid one more iteration is needed as that should be:
> Requires:       sdcc = %{version}-%{release}

FIXED

> As we always want this package and the base package to be in sync.
> 
> Otherwise things look good so with that last change this package can be
> approved. I'll take a look at avrdude next.
> 

Great! Thanks for your help.


Comment 33 Trond Danielsen 2007-03-01 11:11:51 UTC
I was thinking about adding a Fedora specific README file, describing why the
binaries are not installed directly into /usr/bin. But I was wondering if this
is common practice or neccessary?

Comment 34 Hans de Goede 2007-03-01 11:19:25 UTC
(In reply to comment #33)
> I was thinking about adding a Fedora specific README file, describing why the
> binaries are not installed directly into /usr/bin. But I was wondering if this
> is common practice or neccessary?

Yes thats a good idea and it _should_ be common practice :)

Comment 35 Trond Danielsen 2007-03-01 12:18:38 UTC
README.fedora added, and premissions in debuginfo-package fixed.

Updated files at: ftp://open-gnss.org/pub/fedora/sdcc

Comment 36 Hans de Goede 2007-03-01 13:31:16 UTC
Looking good:

Approved by Hans de Goede

Now go get yourself a Fedora Account if you haven't done that already sign the
CLA and then request cvs-extras group member ship. One you've done that I'll be
notified by the account system that someone (you) needs a sponsor and I'll
sponsor you.

When this is completed (IOW you are a cvs-extras member) you can then ask for
CVS-branches as described here:
http://fedoraproject.org/wiki/CVSAdminProcedure

And then get your system ready to import packages and import the package as
described here:
http://fedoraproject.org/wiki/Extras/Contributors


Comment 37 Trond Danielsen 2007-03-01 23:45:19 UTC
New Package CVS Request
=======================
Package Name: sdcc
Short Description: Small Device C Compiler
Owners: trond.danielsen
Branches: FC-5 FC-6
InitialCC: 

Comment 38 Hans de Goede 2007-03-02 06:32:17 UTC
To CVS admin, correction could you make that (iow add me to the initialCC):

New Package CVS Request
=======================
Package Name: sdcc
Short Description: Small Device C Compiler
Owners: trond.danielsen
Branches: FC-5 FC-6
InitialCC: j.w.r.degoede




Comment 39 Dennis Gilmore 2007-03-02 12:52:59 UTC
Done

Comment 40 Jiri Kastner 2015-06-04 14:07:47 UTC
New Package SCM Request
=======================
Package Name: sdcc
Short Description: Small Device C Compiler
Owners: jcapik
Branches: epel7
InitialCC:

Comment 41 Gwyn Ciesla 2015-06-04 15:21:37 UTC
Git done (by process-git-requests).

Comment 42 Jaromír Cápík 2015-06-04 19:06:59 UTC
Hello Jon.

I requested epel7, but the branch doesn't exist.
Could you please re-check?

Thanks,
Jaromir.

Comment 43 Gwyn Ciesla 2015-06-04 19:53:27 UTC
Trying again.  Not in pkgdb.

Comment 44 Gwyn Ciesla 2015-06-04 19:55:14 UTC
Oh. . .use Package Change, not New Package.

Comment 45 Hans de Goede 2016-01-31 12:06:23 UTC
EPEL-7 branch has been created, closing.