Bug 874980 - Review Request: chicken - A compiler for the Scheme programming language
Review Request: chicken - A compiler for the Scheme programming language
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Alec Leamas
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2012-11-09 04:03 EST by Minh Ngo
Modified: 2013-03-25 11:31 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-03-25 11:31:56 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Minh Ngo 2012-11-09 04:03:44 EST
Spec URL: https://raw.github.com/Ignotus/fedora-packages/6779dd2061c252ccf26f525197ece392f38c5cbd/chicken/chicken.spec
SRPM URL: http://kojipkgs.fedoraproject.org//work/tasks/1447/4671447/chicken-4.7.0.6-1.fc17.src.rpm
Description: A compiler for the Scheme programming language
Fedora Account System Username: minh
Comment 1 Minh Ngo 2012-11-09 04:11:38 EST
Oh, I'm sorry. It's a duplicate of the #819919
Comment 2 Fabian Affolter 2012-11-11 06:33:06 EST

*** This bug has been marked as a duplicate of bug 819919 ***
Comment 3 Alec Leamas 2012-12-17 20:09:26 EST
Minh Ngo: Are you still interested in getting chicken-scheme into fedora? The duplicate 819919 has just been closed, non-responsive submitter. I can review if you like.
Comment 4 Minh Ngo 2012-12-18 01:50:36 EST
Yes, I'm
Comment 5 Alec Leamas 2012-12-18 04:30:49 EST
OK, I'll review this one.
Comment 6 Alec Leamas 2012-12-18 08:35:18 EST
OK, here we go! I'm aware that compilers are tricky, so please enlighten me if I'm just wrong... Some initial remarks:

  - The srpm link seems dead (error 404 for me). I rebuilt from spec for now.
  - I cannot find and GPL or MIT licensing in the sources at a quick glance. Could you comment on why the License: is "GPLv2 and BSD and MIT"? (the tests are not part of the binary RPM:s, so test licenses does not apply AFAIK).
  - Here is a -static subpackage. Could you comment on why this is indeed necessary? [5]
  - Are the %{optflags} really in effect as required in [6] ?
  - We have have rpmlint rpath warnings need to be dealt with [1].
  - We have some header files under /usr/include. Shouldn't these go to a  - -devel pkg as well as /usr/lib/libchicken.so? [2]
  - The package Provides: a lot of private libs under /usr/lib/chicken/6 which need to be filtered, see [3] and [4].
  - The package chicken-doc is arched and will thus be duplicated in repos. Shouldn't it be noarch?
  - Use a wildcard as manpage extension (not .gz) as this might change.
  - rpmlint no-manual-page-for-binary can be ignored.
  - The private so-files files  are generating
rmplint devel-file-in-non-devel-package. These can be ignored.

[1] http://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath
[2] http://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages
[3] https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering
[4] https://fedorahosted.org/fpc/ticket/189
[5] http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries
[6] http://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags
Comment 7 Alec Leamas 2012-12-29 04:33:30 EST
Hi! To get this review rolling I need you to reply to my remarks! If anything is unclear (some of the remarks are admittedly somewhat vague) just let me know, and we'll sort it out together.
Comment 8 Minh Ngo 2013-01-14 13:37:58 EST
To Alec:


[ignotus@laptop chicken]$ wget http://code.call-cc.org/stability/4.7.0/chicken-4.7.0.6.tar.gz
--2013-01-14 20:37:10--  http://code.call-cc.org/stability/4.7.0/chicken-4.7.0.6.tar.gz
Resolving code.call-cc.org (code.call-cc.org)... 209.172.33.78
Connecting to code.call-cc.org (code.call-cc.org)|209.172.33.78|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 3425028 (3.3M) [application/x-gzip]
Saving to: `chicken-4.7.0.6.tar.gz'



I can download an archive by this URL. Please check on your side again
Comment 9 Minh Ngo 2013-01-14 13:39:21 EST
Oh, I'm sorry, You have told about SRPM archive :)
Comment 10 Minh Ngo 2013-01-14 13:45:59 EST
What about licenses. Look in the LICENSE file. 

Line 239 - GPL

Line 1 - BSD

Line 12 - Seems like MIT license
Comment 12 Alec Leamas 2013-01-14 15:46:42 EST
Hi Ngo! Nice to hear from you (back from New Year celebrations?)

We have settled the first issue, the links.

As for the LICENSE, the fact that it's mentioned in the LICENSE file isn't enough to get it included; e. g., licenses for test files which are not part of package should not be included the in License: tag. Furthermore, if multiple licenses are indeed used as you propose you must somehow provide a license break-down. [1]

The static archive and package is removed. Fine with me, I assume you have checked that it wasn't essential for using the thing.

Although the changelog states that you have coped with %{optflags} I cannot see that it's used. On the contrary, an example compilaton in the log is like

gcc  -L. -shared -Wl,-R"/usr/lib" srfi-13.import.o -o srfi-13.import.so \
  -lchicken \
  -lm -ldl

Here you have at least two problems: the %{optflags} are missing, and here is also the reason for the bad rpath (see below).

As a starter, you could try adding export CFLAGS="%{optflags}" before the
'make'. It might be enough to include %{optflags}. Use "rpm --eval %optflags" to see what optflags really is, and compare with the build logs to see that they are in effect.

The rpath is trickier, you will need to patch the build script to get rid of that. Run rpmlint yourself, and look for lines like "chicken.i686: E: binary-or-shlib-defines-rpath /usr/bin/csc ['/usr/lib']". You must get rid of those, period.

The -devel package solves the libchicken.so and /usr/include files nicely.

The -doc package is still arched i. e., it's missing Arch: noarch tag. Just add that to the subpackage (to avoid generating different docs for i386/x86_64/arm).

Please change %doc %{_mandir}/man1/cs?.1.gz to %doc %{_mandir}/man1/cs?.1.* - rumor is that gz some day will be replaced w something else.

If you make something like rpm -q --provides -p <your binary rpm> you will see (excerpt)

chicken:
    chicken
    chicken(x86-32)
    chicken.import.so
    csi.import.so
    data-structures.import.so
    extras.import.so
    files.import.so
    foreign.import.so
    irregex.import.so
    libchicken.so.6
  

The problem here is that almost all of these libraries are private and not meant to be used by others. But since they are in the Provides: list, other packages might (and thus eventually will)  link to them causing all sorts of troubles e. g. when upgrading. To avoid this you must filter these Provides: as described in links in comment #6.

This is actually a mess. Perhaps example in [2] could be useful, don't know.

Summary:
 - Fix the rpath
 - Filter the Provides:
 - Either provide a license break-down if needed, or use a single 
   license if this is the outcome of your break-down work.
 - Fix  noarch docs, 
 - Fix.* manpage in %files, 

Don't hesitate to ask! I'm here to help you :)

[1] http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios
[2] http://leamas.fedorapeople.org/spotify/0.8.8/spotify-client.spec
Comment 13 Alec Leamas 2013-01-14 15:53:41 EST
PS. I forgot 'fix %{optflags}' in the summary. I bet you didn't make the same miss, but just in case... DS
Comment 14 Alec Leamas 2013-01-15 10:45:14 EST
Some more random remarks on documentation:
- The LICENSE, NEWS* and README should all be in %doc, preferably in 
  the base package (for LICENSE, this is a must).
- The manpages can go in the base package as well, they are small and needed.
- The build produces a nice html manual. Put that that into the -doc 
  subpackage, it's a shame not packaging it!
Comment 15 Minh Ngo 2013-02-15 02:40:14 EST
Hi again. I'm sorry because of the late answer. I have also celebrated the Vietnamese New Year :).

Seems most of your remarks have been fixed.
SRPM: https://dl.dropbox.com/s/k4ctmvsgp2m1dx3/chicken-4.7.0.6-3.el6.src.rpm?token_hash=AAEObhnw7gQqGbPQMZai2c4x27m58_qiTKdc1t_oEn_aPA&dl=1

SPEC: https://raw.github.com/Ignotus/fedora-packages/3e350102bdc2bdefa738ce0fd744963b760551be/chicken/chicken.spec

Please write me if I have forgotten something.

- Adding LICENSE NEWS* README* files to the base package
- Moving man pages to the base package 
- Adding html manuals into the doc package
- Adding library filter
- Moving include files to the devel package
- Removing the static lib

I know about the problem with %{optflags}. I'm going to fix it today but later.
Comment 16 Alec Leamas 2013-02-15 07:23:40 EST
Hi! Nice to hear from you, but I guessed it was the new year. Welcome back!

License: If you insist on multiple licenses, you must provide a license break-down (links in comment #12). I'm not convinced this is necessary though.

rpaths: have you really got rid of those and checked with rpmlint? The macros in %check also looks *very* strange, what's going on here?

Static lib: Isn't it the buildflag STATICBUILD which enables this? Can't you just disable that instead of building and removing the .a lib? Just a guess from my side.

Please check the provides (build binary rpms and run rpm -q --provide -p <your binary package> and verify that all looks sound. Also verify that you can install the package, since this often becomes problematic when filtering)

Have you checked the build log and verified that %optflags is used?

BuildArch: noarch is still missing for the docs package.
Comment 17 Alec Leamas 2013-03-07 07:42:14 EST
Ping?! This goes real slow... is it that you don't really have time for this, or is it something in the remarks which causes difficulties?
Comment 18 Jessica Jones 2013-03-07 08:51:38 EST
(In reply to comment #17)
> Ping?! This goes real slow... is it that you don't really have time for
> this, or is it something in the remarks which causes difficulties?

I can continue working on this now that I have internet again, if need be.
Comment 19 Alec Leamas 2013-03-07 09:23:41 EST
Which is nice, thanks. Still, this bug (874980) is submitted by Minh Ngo, and he's the one which eventually need to submit the package.

That said, it might be that Minh would appreciate a helping hand now during the review and perhaps also later as a co-maintainer. May I suggest that you approach him by a private email if you you are interested in this?

Also, while writing: there are new filtering guidelines under way at [1]. That draft is voted on right now in the FPC. Given that current filtering guidelines are completely outdated, I think it's a better idea to use this draft to make a working filter setup.

[1] https://fedoraproject.org/wiki/User:Toshio/AutoProvidesAndRequiresFilteringDraft
Comment 20 Alec Leamas 2013-03-18 07:39:27 EDT
Minh: It's now a full month since your last message, so I invoke the stalled review policy [1]. Please state your intentions ASAP.

[1] http://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
Comment 21 Alec Leamas 2013-03-25 11:31:56 EDT
Closing bug: unresponsive submitter as of policy in comment #20.

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