Bug 428603 - Review Request: Falcon - The Falcon Programming Language
Review Request: Falcon - The Falcon Programming Language
Status: CLOSED DUPLICATE of bug 430307
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Michel Alexandre Salim
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-13 23:01 EST by Giancarlo Niccolai
Modified: 2008-01-25 18:04 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-01-25 18:04:03 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Changes to latest Falcon spec file (2.18 KB, patch)
2008-01-19 18:07 EST, Michel Alexandre Salim
no flags Details | Diff
Minor changes to latest Falcon spec file (974 bytes, patch)
2008-01-20 23:23 EST, Michel Alexandre Salim
no flags Details | Diff
Spech wirth Apache2 license and comment. (4.49 KB, text/plain)
2008-01-21 16:34 EST, Giancarlo Niccolai
no flags Details
Done. -j is back to 2 with a comment specifying to keep it as such. (4.64 KB, text/plain)
2008-01-21 17:51 EST, Giancarlo Niccolai
no flags Details

  None (edit)
Description Giancarlo Niccolai 2008-01-13 23:01:01 EST
Spec URL: http://www.falconpl.org/downloads/0.8.7/falcon.spec 
SRPM URL: http://www.falconpl.org/downloads/0.8.7/Falcon-0.8.7-1.src.rpm
Description: Falcon aims to be the next generation scripting language, providing a wide set of programming paradigms, template file processing for web development and document based applications, radical internationalization and application extension support. The package includes the main Falcon core system and the "feather" modules, that are the main modules considered part of the language, as the reflexive compiler, the regex module, the zlib binding and others. Although we've been releasing RPMs for some version now, this is the first try at an official SRPM; please, lend us a hand.
Comment 1 Michel Alexandre Salim 2008-01-14 09:59:00 EST
Will do full review in a bit, but here's the result of running rpmlint on the
package, as well as suggested changes.

Falcon.src: E: invalid-spec-name falcon.spec
-- should be Falcon.spec

Falcon.src: E: non-utf8-spec-file /tmp/Falcon-0.8.7-1.src.rpm.31479/falcon.spec
-- just re-save the spec file with UTF8 character encoding

Falcon.src:31: W: hardcoded-prefix-tag %{prefix}
Falcon.src:37: W: hardcoded-packager-tag Giancarlo
Falcon.src:39: W: unversioned-explicit-provides %{name}
-- Please remove these. Also, release number should be followed by %{?dist} so
that when your package is built on the build server, it is tagged with the
distribution name (.f8, .f9, etc)

Falcon.src:40: W: hardcoded-path-in-buildroot-tag /tmp/%{name}-%{version}
-- see this document:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-b4fdd45fa76cbf54c885ef0836361319ab962473

Falcon.src:64: W: setup-not-quiet
-- add -q to %setup

Falcon.src:65: W: rpm-buildroot-usage %prep rm -rf $RPM_BUILD_ROOT
-- rm -rf should only be in %install and %clean

Falcon.src:72: W: rpm-buildroot-usage %build ./build.sh -p $RPM_BUILD_ROOT/usr
-f /usr
-- use %{_prefix} instead of /usr . I guess $RPM_BUILD_ROOT can't be avoided
here because of the way build.sh is written

Falcon.src:118: E: hardcoded-library-path in /usr/lib
-- in %files, use %{_bindir}, %{_includedir}, %{_datadir} and %{_libdir}

Falcon.src: E: no-changelogname-tag
-- add a %changelog section at bottom with entries in this format:
* Mon Jan 14 2008 Giancarlo Niccolai <gc@falconpl.org> -- 0.8.7-1
- Initial Fedora package

Falcon.src: W: invalid-license Falcon License
-- Might have to clear this on the fedora-legal mailing list; it is not a
pre-approved license. How different is it from the Apache 2.0 license?
See http://fedoraproject.org/wiki/Packaging/LicensingGuidelines
Comment 2 Giancarlo Niccolai 2008-01-14 10:21:20 EST
Thank you very much.

Just to give you a short insight; the rationale of the license is here.
http://www.falconpl.org/?page_id=license_comment

It differs from Apache in the sense that it is tailored on a "compiler" instead 
than on a "server". The idea is that to grant freedom of usage in any 
application while still protecting the openness of true derivative works, as 
i.e. revisions of the language itself.

We are willing (actually glad) to submit the license to any certification 
authority; we have also started OSI process (I am getting a legal compliance 
analisys). We'll post it to the suggested list; thanks for the pointer.

Comment 3 Giancarlo Niccolai 2008-01-19 11:45:16 EST
I have cleared all the pointed out issues. 
I am sorry I can't test the RPMLINT output again, but it seems I am not able to
install a runnable version of rpmlint on my FC6 server.

The URLs are the same except for the case of the Falcon.spec:

Spec URL: http://www.falconpl.org/downloads/0.8.7/Falcon.spec 
SRPM URL: http://www.falconpl.org/downloads/0.8.7/Falcon-0.8.7-1.src.rpm

I have submitted a request about Falcon license to fedora-legal-list. If any
issue raises, I may use double licensing. I.e. GPL is way more restrictive than
Falcon license, so I would have no problem releasing Falcon in both GPL and
Falcon license for FC (just, users would pick Falcon License regularly, I
suppose ;-).

I am waiting for another review.
Comment 4 Michel Alexandre Salim 2008-01-19 18:07:17 EST
Created attachment 292278 [details]
Changes to latest Falcon spec file

Several things:
- the XXXXXX in the call to mktemp lets you do multiple builds of the same
package at the same time, by generating a randomized 6-digit number; keep it

- One more use of /usr; I substituted %{_prefix} for it
- %{buildroot} is actually fine; to use that or $RPM_BUILD_ROOT is just
  a matter of style
- First attempt at a fixed file listing. 
- Your package builds fine on x86_64, but there is currently no way to override

  the location of library files (on x86_64 and ppc64, Fedora expects the
  libraries to be in /usr/lib64, which is what %{_libdir} expands to). Fix
  build.sh to allow the library path to be passed as argument?
- Also make it possible to override CFLAGS and CXXFLAGS with the content of
  $RPM_OPT_FLAGS
- (this one is optional) While you're at it, would be nice if the -p directive
  to set where the package is to be installed can be passed only at install
  time, rather than at boot time (similar to how most Makefiles asks you for
the
  PREFIX (your final prefix) first, and the DESTDIR is only needed when
  installing)
- falcon-conf -L reports the wrong library path: it reports the value of -p
  instead of -f
- Do you really Requires: gcc at runtime? If anything it should be gcc-c++.
  If you mean BuildRequires:, gcc and gcc-c++ are standard build requirements
  that do not have to be explicitly listed

Feel free to rearrange the files between the main package and -devel. The idea
is that files needed by people who program in Falcon goes in -devel and files
needed to run pre-existing Falcon scripts in the main package.
Comment 5 Giancarlo Niccolai 2008-01-20 07:07:13 EST
I have seen the splitting to devel. This is OK, thanks a lot. I was a bit
helpless on that, sorry.

I will add the %{_lib} prefix, but it may take some time; should be done in a
couple of days.

As for C/CXX Flags, is it ok to read them from the environ and forward them to
CMAKE, or should I get them from the command line too?

I don't need gcc as run depends, sorry, I just need stdc++. Do I have to add it
or is that also a default dependency?

-- Michel, I have integrated your patch directly in our SVN, thanks. If you
wish, we can list you as contributor; just send me a mail with your contact
(i.e. your home page). Also, a professional RPM packager would be a great
addition to our (starting now) community. Would you wish to join, have a look on
our site. TIA.

 
Comment 6 Michel Alexandre Salim 2008-01-20 10:39:55 EST
You can read the flags from the environ -- within RPM's environment, it's
defined as $RPM_OPT_FLAGS. You can do something like CXXFLAGS=$RPM_OPT_FLAGS
./build.sh or something similar, so you don't have to make the build scripts
RPM-specific.

RPM will pick up runtime dependencies automatically, most of the time. Requires:
is normally left out of the spec file, unless you have a runtime requirement
that is not immediately present in any binaries (including libraries)

$ ldd /usr/bin/falcon
        linux-vdso.so.1 =>  (0x00007fff743fe000)
        libfalcon_engine.so.1 => /usr/lib/libfalcon_engine.so.1 (0x000000309f600000)
        libdl.so.2 => /lib64/libdl.so.2 (0x0000003bbfe00000)
        libstdc++.so.6 => /usr/lib64/libstdc++.so.6 (0x0000003bd3800000)
        libm.so.6 => /lib64/libm.so.6 (0x0000003bbfa00000)
        libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x0000003bcf000000)
        libc.so.6 => /lib64/libc.so.6 (0x0000003bbf600000)
        /lib64/ld-linux-x86-64.so.2 (0x0000003bbe400000)

$ rpm -q --requires Falcon
/bin/sh  
/bin/sh  
/bin/sh  
/usr/bin/falcon  
bash        -\
binutils     |- these are there only because they are manually added
gcc         -/
libc.so.6()(64bit)  
libc.so.6(GLIBC_2.2.5)(64bit)  
libc.so.6(GLIBC_2.3)(64bit)  
libdl.so.2()(64bit)  
libdl.so.2(GLIBC_2.2.5)(64bit)  
libfalcon_engine.so.1()(64bit)  
libgcc_s.so.1()(64bit)  
libgcc_s.so.1(GCC_3.0)(64bit)  
libm.so.6()(64bit)  
libm.so.6(GLIBC_2.2.5)(64bit)  
libstdc++.so.6()(64bit)  
libstdc++.so.6(CXXABI_1.3)(64bit)  
libstdc++.so.6(GLIBCXX_3.4)(64bit)  
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rtld(GNU_HASH) 
Comment 7 Giancarlo Niccolai 2008-01-20 15:28:02 EST
I think I have cleared the issues you were presenting. I have splitted the
package in source, binary and devel, with correct file list including the
development support binaries, data, scripts and manuals.

In the meanwhile, we have released the official 0.8.8, which we think is stable
enough to be ready for packaging. It can be downloaded here:

Spec URL: http://www.falconpl.org/downloads/0.8.8/Falcon.spec 
SRPM URL: http://www.falconpl.org/downloads/0.8.8/Falcon-0.8.8-1.src.rpm

TIA for your help.
Comment 8 Michel Alexandre Salim 2008-01-20 23:23:19 EST
Created attachment 292313 [details]
Minor changes to latest Falcon spec file

Almost there. There were some build dependencies (cmake, pcre-devel,
zlib-devel) that I did not notice earlier (they were already installed on my
system -- and pcre is new in 0.8.8?).

You can use %{_lib} instead of %{_libdir}#%{_prefix} (though that's a nice
trick -- haven't seen that before).

Builds fine on my machine for i386 and x86_64, however, on the Fedora build
server (running mock) I get these errors. Any idea?

i386:  http://koji.fedoraproject.org/koji/getfile?taskID=361213&name=build.log
ppc64: http://koji.fedoraproject.org/koji/getfile?taskID=361214&name=build.log

Once the build issues are tracked down, everything else is ready to go (apart
from license). If the Falcon license is more liberal than Apache, perhaps you
could state the license as Apache, and add a note on the spec file (above the
License line) referring to the included LICENSE file?
Comment 9 Giancarlo Niccolai 2008-01-21 16:34:48 EST
Created attachment 292416 [details]
Spech wirth Apache2 license and comment.
Comment 10 Giancarlo Niccolai 2008-01-21 16:41:19 EST
Sorry, the attachment went out without comment.

It's the Falcon.spec updated with the license exception to apache2.

About the dependency, yes it has been added in the last version, sorry.

About the errors with mock. No idea: there is no C error (only warnings, and
also quite strict ones), but the error seems generated by this:

/bin/sh:
/builddir/build/BUILD/Falcon-0.8.8/devel/release/build/share/man/man1/falconeer.fal.1.gz:
No such file or directory

but there is no hardcoded /builddir anywhere in the spec nor in the build files.
couldn't that be related with the very long "mock won't build anything" thread
in the devel-list?
Comment 11 Michel Alexandre Salim 2008-01-21 17:00:06 EST
Ah, good, the weird characters in the Italian version of the description is
fixed as well (was about to mention it -- rpmlint apparently only checks the
English description).

/builddir/build/BUILD/... is just where mock unpacks the sources, so that's
fine. I have a feeling it's a race condition in the build script -- the build
server was using -j 8, and the original spec you posted had -j 2 fixed.

I'm trying it now with -j 2; if it works, I'll approve the package -- could you
put an additional comment above the -j 2 line that the reason it's hardcoded is
because higher -j settings have problems?
Comment 12 Michel Alexandre Salim 2008-01-21 17:34:34 EST
Build succeeded -- APPROVED
Comment 13 Michel Alexandre Salim 2008-01-21 17:35:21 EST
Build succeeded -- APPROVED. Just change the sed hack back to -j 2 and add the
comment.
Comment 14 Giancarlo Niccolai 2008-01-21 17:51:03 EST
Created attachment 292424 [details]
Done. -j is back to 2 with a comment specifying to keep it as such.

-j is back to 2 with a comment specifying to keep it as such.
This is a complete replacement of previous .spec files.

Thank you all for your support!
Comment 15 Michel Alexandre Salim 2008-01-21 21:00:34 EST
OK. my part in the review is done now; the next steps are outlined here:

http://fedoraproject.org/wiki/PackageReviewProcess
http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure

Feel free to add me to the InitialCc (or not), and close the bug as CLOSED - 
NEXTRELEASE once you've done a build using Koji.
Comment 16 Michel Alexandre Salim 2008-01-25 18:04:03 EST

*** This bug has been marked as a duplicate of 430307 ***

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