Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
Summary: Review Request: sdcc - Small Device C Compiler Product: Fedora Extras Version: devel Platform: All OS/Version: Linux Status: NEW Severity: normal Priority: normal Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: trond.danielsen@gmail.com QAContact: fedora-package-review@redhat.com
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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From trond.danielsen@gmail.com 2007-02-01 12:20 EST ------- 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From blt@iastate.edu 2007-02-01 18:51 EST ------- This would be very, very great to have in the extras or even base tree.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
jeff@ocjtech.us changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jeff@ocjtech.us
------- Additional Comments From jeff@ocjtech.us 2007-02-02 00:24 EST ------- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From rc040203@freenet.de 2007-02-02 00:50 EST ------- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From jeff@ocjtech.us 2007-02-02 01:09 EST ------- (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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From rc040203@freenet.de 2007-02-02 02:03 EST ------- (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From trond.danielsen@gmail.com 2007-02-02 04:34 EST ------- (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):
The "script-without-shebang" errors can be fixed with this:
find . -type f -name *.c | xargs chmod a-x
FIXED
- 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?
- 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.
- 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.
- The devel package doesn't own "%{_datadir}/sdcc".
Devel package removed, see 7.
- 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
- 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.
- 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@gnu.org.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From trond.danielsen@gmail.com 2007-02-02 04:36 EST ------- (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From trond.danielsen@gmail.com 2007-02-02 04:41 EST ------- (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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From trond.danielsen@gmail.com 2007-02-02 04:55 EST ------- (In reply to comment #7)
(In reply to comment #3)
- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From rc040203@freenet.de 2007-02-02 05:12 EST ------- (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From trond.danielsen@gmail.com 2007-02-02 06:17 EST ------- (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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
trond.danielsen@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO| |177841 nThis| |
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From rc040203@freenet.de 2007-02-05 05:15 EST ------- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From trond.danielsen@gmail.com 2007-02-05 06:19 EST ------- (In reply to comment #13)
- 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
- 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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From hellwolf.misty@gmail.com 2007-02-12 01:53 EST ------- == 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
j.w.r.degoede@hhs.nl changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |j.w.r.degoede@hhs.nl
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-02-24 06:14 EST ------- 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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From trond.danielsen@gmail.com 2007-02-25 18:23 EST ------- 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/
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From rc040203@freenet.de 2007-02-26 02:40 EST ------- Created an attachment (id=148782) --> (https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=148782&action=vie...) Remove bogus compiler flags
This patch is supposed to resolve the "package doesn't acknowledge RPM_OPT_FLAGS" issues mentioned in comment #6
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From rc040203@freenet.de 2007-02-26 03:21 EST ------- Created an attachment (id=148783) --> (https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=148783&action=vie...) 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-02-26 04:49 EST ------- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From trond.danielsen@gmail.com 2007-02-26 05:08 EST ------- Patch from #18 applied and spec file from #19 rebuilt. Updated SRPM and spec file at ftp://open-gnss.org/pub/fedora/sdcc/
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-02-26 10:20 EST ------- 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 :)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From rc040203@freenet.de 2007-02-26 11:06 EST ------- (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=:
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-02-26 11:42 EST ------- Yes, that will work too.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
j.w.r.degoede@hhs.nl changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |j.w.r.degoede@hhs.nl Flag| |fedora-review?
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-02-26 16:01 EST ------- 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/*
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From trond.danielsen@gmail.com 2007-02-27 05:49 EST ------- (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/
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-02-27 16:06 EST ------- 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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From rc040203@freenet.de 2007-02-27 21:46 EST ------- (In reply to comment #27)
Looks good,
A few last issues:
- 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.
- Remove the empty %doc from the "%files src"
- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-02-28 04:18 EST ------- (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From trond.danielsen@gmail.com 2007-02-28 06:26 EST ------- (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@gmail.com - 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-02-28 06:46 EST ------- (In reply to comment #30)
and updated sdcc according to the comments; snippet from changelog:
- Wed Feb 28 2007 Trond Danielsen trond.danielsen@gmail.com - 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From trond.danielsen@gmail.com 2007-02-28 07:01 EST ------- (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From trond.danielsen@gmail.com 2007-03-01 06:11 EST ------- 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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-03-01 06:19 EST ------- (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 :)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From trond.danielsen@gmail.com 2007-03-01 07:18 EST ------- README.fedora added, and premissions in debuginfo-package fixed.
Updated files at: ftp://open-gnss.org/pub/fedora/sdcc
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
j.w.r.degoede@hhs.nl changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO|163776, 177841 | nThis| | Flag|fedora-review? |fedora-review+
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-03-01 08:31 EST ------- 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
trond.danielsen@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
------- Additional Comments From trond.danielsen@gmail.com 2007-03-01 18:45 EST ------- New Package CVS Request ======================= Package Name: sdcc Short Description: Small Device C Compiler Owners: trond.danielsen@gmail.com Branches: FC-5 FC-6 InitialCC:
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
------- Additional Comments From j.w.r.degoede@hhs.nl 2007-03-02 01:32 EST ------- 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@gmail.com Branches: FC-5 FC-6 InitialCC: j.w.r.degoede@hhs.nl
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
dennis@ausil.us changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |fedora-cvs+
------- Additional Comments From dennis@ausil.us 2007-03-02 07:52 EST ------- Done
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: sdcc - Small Device C Compiler
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226795
trond.danielsen@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |NEXTRELEASE
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla