Bug 448310 - Review Request: elice - Elice is a PureBasic to c++ translator / compiler
Review Request: elice - Elice is a PureBasic to c++ translator / compiler
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ignacio Vazquez-Abrams
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 448311 448312 448313
  Show dependency treegraph
 
Reported: 2008-05-25 15:31 EDT by Hans de Goede
Modified: 2008-06-16 16:06 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-06-16 16:05:53 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ivazqueznet: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2008-05-25 15:31:57 EDT
Spec URL: http://people.atrpms.net/~hdegoede/elice.spec
SRPM URL: http://people.atrpms.net/~hdegoede/elice-0.0-0.1.svn257.fc9.src.rpm
Description:
Elice is a PureBasic (http://www.purebasic.com/) to c++ translator / compiler.
Elice was written to have a Free compiler for Lost Labyrinth
(http://www.lostlabyrinth.com/) an adventure game written in PureBasic, as such
it currently only supports a subset of PureBasic and it provides 2 tools for
packing Lost Labyrinth resources: lostlabyrinth_pack_sound and
lostlabyrinth_pack_graphics.
Comment 1 Ignacio Vazquez-Abrams 2008-05-31 05:34:08 EDT
- Summary: A partial PureBasic to C++ cross-compiler
? Generation commands should use svn export instead
- The ruby BR is covered by ruby-racc
? "C++" case in %description
? "make: bzr: Command not found" - not a blocker, but it is ugly (I thought it
was pulled from svn...)
- I don't like that the lostlaby patches are in here instead of the
lostlabyrinth SRPM
. Noted that 2 lostlabyrinth-specific executables are included
??? HOLY CRAP! Is that really all the font data from Vera.ttf included in the
script?!
? No license reference in many of the source files
? Tests aren't run in %check
Comment 2 Hans de Goede 2008-05-31 15:37:39 EDT
(In reply to comment #1)
> - Summary: A partial PureBasic to C++ cross-compiler
> ? Generation commands should use svn export instead

I don't use svn that often, but indeed export is much better in this case

> - The ruby BR is covered by ruby-racc

Only implicit not (through /usr/bin/ruby and ruby(abi) = 1.8) not explicit and
the ruby guidelines:
http://fedoraproject.org/wiki/Packaging/Ruby
say:
"Ruby packages must require ruby at build time with a BuildRequires: ruby"

> ? "C++" case in %description

Fixed

> ? "make: bzr: Command not found" - not a blocker, but it is ugly (I thought it
> was pulled from svn...)

I know, I guess the elice author uses bzr for its own internal version tracker
and then from time to time submits his work to Lost Labyrinths svn. Nothing I
can do here really.

> - I don't like that the lostlaby patches are in here instead of the
> lostlabyrinth SRPM

Oh, those are no longer needed, they have been integrated into Lost Labyrinth
2.9.2 -> dropped.

> . Noted that 2 lostlabyrinth-specific executables are included

Yip, as said in the description, this really is only meant for building Lost
Labyrinth

> ??? HOLY CRAP! Is that really all the font data from Vera.ttf included in the
> script?!

Erm, yes it would seem so, I will contact upstream about this asking them to
just use a file instead.

> ? No license reference in many of the source files

The licensing for all relevant files (including a file list) is explained in the
file titled COPYING (no this is not just a copy of the GPL).

> ? Tests aren't run in %check

Good catch! Fixed.

Here is a new version with most issues fixed:
Spec URL: http://people.atrpms.net/~hdegoede/elice.spec
SRPM URL: http://people.atrpms.net/~hdegoede/elice-0.0-0.2.svn257.fc9.src.rpm
Comment 3 Hans de Goede 2008-06-12 02:30:53 EDT
Erm, ping?
Comment 4 Ignacio Vazquez-Abrams 2008-06-12 12:25:55 EDT
Sorry, I missed the links at the bottom.

- Release should be 0%{?dist}.2.svn%{svn_revision}
? make check runs, but I'm not convinced it does anything

Other than those, APPROVED.
Comment 5 Hans de Goede 2008-06-12 13:16:37 EDT
(In reply to comment #4)
> Sorry, I missed the links at the bottom.
> 
NP.

> - Release should be 0%{?dist}.2.svn%{svn_revision}

Erm, where did you get that from ??
[hans@localhost ~]$ rpm -qa | grep svn
qimageblitz-0.0.4-0.4.svn706674.fc9.x86_64
kdnssd-avahi-devel-0.1.3-0.6.20080116svn.fc9.x86_64
NetworkManager-vpnc-0.7.0-0.7.7.svn3502.fc9.x86_64
libflashsupport-000-0.3.svn20070904.i386
libnetfilter_conntrack-0.0.89-0.1.svn7356.fc9.x86_64
<much more snipped>

[hans@localhost ~]$ rpm -qa | grep cvs
mdbtools-devel-0.6-0.4.cvs20051109.fc9.x86_64
xmldb-api-sdk-0.1-0.2.20011111cvs.1jpp.2.fc9.x86_64
xu4-1.1-0.4.cvs20070510.fc9.x86_64
<much more snipped>

Also from:
http://fedoraproject.org/wiki/Packaging/DistTag

"Basically, follow the ["Packaging/NamingGuidelines"] for how to set the value
for Release, then append %{?dist} to the end." Notice the "append %{?dist} to
the end."

> ? make check runs, but I'm not convinced it does anything

It does I wasn't convinced either and I checked, its just very quiet when
everything goes ok.
Comment 6 Hans de Goede 2008-06-13 05:47:23 EDT
I've just read a mail from upstream that they are making available tarballs now,
so here is a new version which uses a tarbal instead of an svn snapshot:

http://people.atrpms.net/~hdegoede/elice.spec
http://people.atrpms.net/~hdegoede/elice-0.258-1.fc10.src.rpm
Comment 7 Ignacio Vazquez-Abrams 2008-06-13 10:09:03 EDT
(In reply to comment #5)
> Also from:
> http://fedoraproject.org/wiki/Packaging/DistTag
> 
> "Basically, follow the ["Packaging/NamingGuidelines"] for how to set the value
> for Release, then append %{?dist} to the end." Notice the "append %{?dist} to
> the end."

Interesting. It could be interpreted as conflicting with
https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Minor_release_bumps_for_old_branches
then.

Oh well, a moot point now.

APPROVED
Comment 8 Hans de Goede 2008-06-13 16:55:35 EDT
New Package CVS Request
=======================
Package Name:      elice
Short Description: Elice is a PureBasic to c++ translator / compiler
Owners:            jwrdegoede
Branches:          F-8 F-9
InitialCC:
Cvsextras Commits: yes

Comment 9 Kevin Fenzi 2008-06-16 12:03:29 EDT
cvs done.
Comment 10 Hans de Goede 2008-06-16 16:05:53 EDT
Imported and build. For F-8 and F-9 Í've requested tagging into the
buildroot-verride tag, once lostlabyrinth is build I will push an elice update
together with lostlaby.
Comment 11 Hans de Goede 2008-06-16 16:06:23 EDT
p.s.

Thanks for the review!

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