Bug 452107 - Review Request: cfdg - Context Free Design Grammar
Review Request: cfdg - Context Free Design Grammar
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity low
: ---
: ---
Assigned To: Terje Røsten
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 452108
  Show dependency treegraph
 
Reported: 2008-06-19 09:31 EDT by Gwyn Ciesla
Modified: 2008-10-30 08:54 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-30 08:54:34 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
terjeros: fedora‑review+
dennis: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Gwyn Ciesla 2008-06-19 09:31:43 EDT
SPEC: http://zanoni.jcomserv.net/fedora/cfdg/cfdg.spec
SRPM: http://zanoni.jcomserv.net/fedora/cfdg/cfdg-2.1-1.fc9.src.rpm

Context Free is a program that generates images from written instructions 
called a grammar. The program follows the instructions in a few seconds to 
create images that can contain millions of shapes.
Comment 1 Jason Tibbitts 2008-06-22 18:55:09 EDT
I note the proper compiler flags aren't used; most things seem to be compiled
with -Isrc-common -Isrc-unix -Iobjs -Isrc-agg/include -Isrc-common/agg-extras
-O3  -c 

There seems to be the occasional cc call, probably from yacc, that includes the
regular optflags plus all of the above.  That's probably why the debuginfo
package includes only the yacc-generated bits.

There are also several patches included; it would be good to get comments
including at least their upstream status.
Comment 2 Gwyn Ciesla 2008-06-25 11:02:51 EDT
Patches sent upstream (for gcc43 compat), except for the nostrip patch.  Can you
suggest a way to enforce the proper compiler flags?  I've played around with
setting CFLAGS and CPPFLAGS and not having much success.
Comment 3 Jason Tibbitts 2008-06-25 13:21:51 EDT
> Patches sent upstream (for gcc43 compat), except for the nostrip patch.

Please document that in the spec.  See 
http://fedoraproject.org/wiki/Packaging/PatchUpstreamStatus

> Can you suggest a way to enforce the proper compiler flags?

Patch the CPPFLAGS define in the Makefile, perhaps?

Also, does this package include its own version of agg?  I think agg is already
in the distro.  If that's the case, this package should use the system version.
Comment 4 Gwyn Ciesla 2008-06-30 09:09:52 EDT
Still playing with the compiler flags.

Just got confirmation from upstream that cfdg needs the bundled agg to run, as
upstream agg has thusfar been unresponsive to their patches.
Comment 5 Rakesh Pandit 2008-08-16 16:30:29 EDT
@Jon

Any updates regarding flags ?

This request is stalled for long and needs an update soon.
Comment 6 Gwyn Ciesla 2008-08-18 13:51:44 EDT
Not so far.  Life intervened a bit, I'll keep playing with it.
Comment 8 Terje Røsten 2008-10-14 15:07:49 EDT
Still build optflags issue and mktemp issue:
 
 http://koji.fedoraproject.org/koji/getfile?taskID=880082&name=build.log

full koji ref:

 http://koji.fedoraproject.org/koji/taskinfo?taskID=880079
Comment 9 Gwyn Ciesla 2008-10-14 16:18:18 EDT
Patched for the mktemp issue, still at a loss as to how to correct the optflags issue.

Any suggestions?

SPEC: http://zanoni.jcomserv.net/fedora/cfdg/cfdg.spec
SRPM: http://zanoni.jcomserv.net/fedora/cfdg/cfdg-2.1-3.fc9.src.rpm
Comment 10 Terje Røsten 2008-10-14 17:26:36 EDT
Add this patch:

--- ContextFreeSource2.1.orig/Makefile  2007-04-29 07:51:20.000000000 +0200
+++ ContextFreeSource2.1/Makefile       2008-10-14 23:12:11.000000000 +0200
@@ -102,8 +102,9 @@
 # Rules
 #

+OPTFLAGS = -03
 CPPFLAGS += $(patsubst %,-I%,$(INC_DIRS))
-CPPFLAGS += -O3
+CPPFLAGS += $(OPTFLAGS)
 #CPPFLAGS += -ggdb

 $(OBJ_DIR)/%.o : %.cpp

Aand change

make CFLAGS+="$RPM_OPT_FLAGS"
 to
make OPTFLAGS="%{optflags}"

BTW: don't mix $ style and % style macros.

This

mkdir -p %{buildroot}%{_bindir}
install -m 755 cfdg %{buildroot}%{_bindir}/cfdg

could be one line:

install -D -m 755 cfdg %{buildroot}%{_bindir}/cfdg

Add smp_flags to make.

Very picky:

-create. should be - Create.

 http://www.contextfreeart.org/index.html

->  http://www.contextfreeart.org/
Comment 11 Gwyn Ciesla 2008-10-15 09:10:56 EDT
Wow, thanks, your suggestions are more details than some people's instructions. :)

SPEC: http://zanoni.jcomserv.net/fedora/cfdg/cfdg.spec
SRPM: http://zanoni.jcomserv.net/fedora/cfdg/cfdg-2.1-4.fc9.src.rpm

All addressed/applied.
Comment 12 Terje Røsten 2008-10-20 04:12:50 EDT
Review Guidelines MUST items:
- [OK] rpmlint output:
- [OK] package name
- [OK] %{name}.spec
- [OK] Packaging Guidelines
- [-] Licensing Guidelines
  GPLv2+ is correct, however some files:
   src-common/test* are under
   Academic Free License version 2.0
  Comment? Contact upstream.

- [OK] License Field in spec
- [-] License text in %doc
  Seems like LICENSE.txt is missing the "and later"
  which is present in files.

- [OK] Spec file in en_US
- [OK] legible spec file
- [OK] source matches upstream
 md5sum ContextFreeSource2.1.tgz*
 477242e74c4f953ceca7bf06e944a46e  ContextFreeSource2.1.tgz
 477242e74c4f953ceca7bf06e944a46e  ContextFreeSource2.1.tgz.rpm

- [OK] compiles successfully
 http://koji.fedoraproject.org/koji/taskinfo?taskID=889881

- N/A  %find_lang
- N/A  shared libs
- N/A  not relocatable
- [OK] directory ownership
- [OK] no duplicate files in %files
- [OK] proper permissions on files, %defattr present
- [OK] %clean section cleans %{buildroot}
- [OK] consistently uses macros
- [OK] package contains code
- N/A  large docs
- [OK] %doc files do not affect runtime behaviour
- N/A  header files in -devel
- N/A  static libs in -static
- N/A  foo.pc files
- N/A  libfoo.so.1.1
- N/A  no devel package
- N/A  no .la archives
- N/A desktop file
- [OK] Does not own files/dirs owned by other packages
- [OK] %install cleans out %{buildroot} first
- [OK] all filenames are valid ASCII and thus UTF-8
- N/A Scriptlets

Summary:

 everything seems fine except some licenses issues:
  - a lot of files are missing license headers
  - there are at least two licenses here GPLv2+ and  
    Academic Free License version 2.0
  - LICENSE.txt seems to be GPLv2.
 
 I would recommend to contact upstream about these issues.
 
 Pedantic:
  - remove extra space in version and release tag.
Comment 13 Gwyn Ciesla 2008-10-20 09:03:49 EDT
Actually, it was my understanding that not all files must have license headers, nor must LICENSE.txt include 'and later.  I'll check on the Academic Free License bits.
Comment 14 Terje Røsten 2008-10-20 13:33:59 EDT
It's not a blocker, however it would be nice of upstream fixed it.
Comment 15 Gwyn Ciesla 2008-10-20 13:40:10 EDT
I'm actually in conversation with them how.  They'll be re-licensing those bits GPLv2+ as well, I'm waiting to hear when the release with this well be.
Comment 16 Terje Røsten 2008-10-20 13:51:25 EDT
These licenses issues tend to show up in Fedora reviews, it takes time and a bit of pain, however in the long run we are making the world a better place.

Thanks for your contribution!
Comment 17 Gwyn Ciesla 2008-10-20 13:59:18 EDT
(In reply to comment #16)
> These licenses issues tend to show up in Fedora reviews, it takes time and a
> bit of pain, however in the long run we are making the world a better place.
> 
+1
Comment 18 Gwyn Ciesla 2008-10-22 09:10:19 EDT
Next release, with this correction, will be in +-1 month.  Should we go ahead with a modified tarball, or wait for the new release?
Comment 19 Terje Røsten 2008-10-24 11:17:59 EDT
Sorry for delay. It's up to you, you decide.
Comment 20 Gwyn Ciesla 2008-10-24 11:35:31 EDT
I'm happy with a modified tarball:

SPEC: http://zanoni.jcomserv.net/fedora/cfdg/cfdg.spec
SRPM: http://zanoni.jcomserv.net/fedora/cfdg/cfdg-2.1-5.fc9.src.rpm
Comment 21 Terje Røsten 2008-10-24 12:00:18 EDT
Great, I can't find any issues with the update package:

 APPROVED
Comment 22 Gwyn Ciesla 2008-10-24 12:06:30 EDT
Thanks!

New Package CVS Request
=======================
Package Name: cfdg
Short Description: Context Free Design Grammar
Owners: limb
Branches: F-9
InitialCC:
Comment 23 Dennis Gilmore 2008-10-27 00:44:24 EDT
CVS Done
Comment 24 Fedora Update System 2008-10-27 08:58:44 EDT
cfdg-2.1-5.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/cfdg-2.1-5.fc9
Comment 25 Fedora Update System 2008-10-30 08:54:31 EDT
cfdg-2.1-5.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

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