[Bug 758166] New: [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

Summary: [EPEL] - Review Request -- thrift 0.6.1

https://bugzilla.redhat.com/show_bug.cgi?id=758166

Summary: [EPEL] - Review Request -- thrift 0.6.1
           Product: Fedora EPEL
           Version: el5
          Platform: Unspecified
        OS/Version: Unspecified
            Status: NEW
          Severity: unspecified
          Priority: unspecified
         Component: Package Review
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]
         QAContact: [e-mail]
                CC: [e-mail]
   Estimated Hours: 0.0
    Classification: Fedora
      Story Points: ---
              Type: ---

Package Review Request: thrift 0.6.1

SPEC: http://nmarques.fedorapeople.org/packages/thrift/thrift.spec
SRPM: http://nmarques.fedorapeople.org/packages/thrift/thrift-0.6.1-0.src.rpm

Description:
------------
A software framework for scalable cross-language services development. It 
combines a powerful software stack with a code generation engine to build
services that work efficiently and seamlessly between C++, Java, C#, Python, 
Ruby, Perl, PHP, Objective C/Cocoa, Smalltalk, Erlang, Objective Caml, and
Haskell.

Packager Notes:
---------------
Target Platforms: EL5/EL6

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #1 from Nelson Marques  2011-12-02 07:19:44 EST ---
I have updated the .spec file and SRPM, here's the new link for the SRPM (spec
remains the same).

SRPM:
http://nmarques.fedorapeople.org/packages/thrift/thrift-0.6.1-1.el5.src.rpm

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

Tom "spot" Callaway  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #2 from Tom "spot" Callaway  2011-12-02 13:42:36 EST ---
Okay. There are quite a few things to work on here:

* Please start your release counter at 1, not 0 (unless it is some sort of
pre-release package as documented in the guidelines)
* There is no need to have this section at all:

# -- package information
%define _name           thrift
%define _version        0.6.1
%define _release        1
%define _group          Development/Libraries
# -- filesystem information
%define _prefix         /usr
# -- debug package

Just populate the Name:, Version:, Release:, Group: fields, as those will then
automatically define %{name}, %{version}, %{release}, %{group}, respectively.
You also should never need to override _prefix.

* The BuildRoot is not one of the approved choices, see:
https://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#Distribution_speci...

* Don't use 0%{?rhel_version} for conditionals. Use 0%{?rhel} (as described in
https://fedoraproject.org/wiki/Packaging:DistTag)

* Explicit Requires should be for foo = %{version}-%{release}, not just foo =
%{version}.

* The Group tag would not be identical for all subpackages. Not that it matters
very much, to be honest, that tag is not required in RHEL 6 or newer, but if
you're going to do it, please be more accurate.

* Also, the %description would not be identical for all subpackages. Please
make them correct and applicable for each subpackage entry.

* Please don't double-up config options to configure on a single line. It makes
it less obvious what is happening there, and more complicated to conditionalize
or comment out in the future.

* The %{?jobs:-j%{jobs}} syntax isn't valid, replace it with %{?_smp_mflags}.

* %defattr(-,root,root) should be %defattr(-,root,root,-)

* For EL-5 or older, packages containing pkgconfig(.pc) files must Requires:
pkgconfig (for directory ownership and usability).

* I'm not sure why you have split the glib library into a subpackage. Is this
to minimize dependencies on the main binary?

* The LICENSE file must be included as %doc as part of every possible install
transaction. This means that if a package with LICENSE is pulled in as a
dependency of another subpackage, it does not need to be present, but in the
case of items like the glib, perl, python subpackages, if they can be installed
without the -libs subpackage, then they need to have LICENSE as %doc.

* You use both $RPM_BUILD_ROOT and %{buildroot}. Pick one and use it
consistently throughout. I recommend %{buildroot}.

* Is it really necessary to use %{__make} and %{__cp} instead of simply "make"
or "cp"?

* You still have FIXME - PHP REQUIRES

* Does make check not work for older releases than RHEL 7?

* Is there a reason this package isn't planned for Fedora?

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

Tom "spot" Callaway  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|[e-mail]    |[e-mail]

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #3 from Nelson Marques  2011-12-05 04:58:12 EST ---
Tom,

Updated as you suggested and bumped release to 3.
Regarding Fedora: I don't mind maintaining it for Fedora as long as it's the
same version. There's a 0.7.0 available already for quite some time, but I've
had a few reports that there's a few glitches. So could we keep 0.6.1 on Fedora
until I have confirmation that some of the glitches were fixed ?

NM

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #4 from Tom "spot" Callaway  2011-12-05 09:55:57 EST ---
I don't see why not, although, you should be prepared for users to file bugs
that you are not at the latest version. :)

* To support Fedora, you'll want to make sure that all the conditionals are
valid for Fedora (where %{rhel} is NULL) (and probably also enable the
functionality that Fedora has but RHEL does not, like csharp, erlang, haskell,
java, ruby).

* I think the php subpackage needs a copy of LICENSE too.

* You will also want to add the appropriate php requires, see:
https://fedoraproject.org/wiki/Packaging:EPEL#PHP_ABI_Check_Handling

* This conditional is weird:

%if %{_arch} == x86_64
Requires:       perl(Math::BigInt)
%endif

Just Requires: perl(Math::BigInt) and be done with it. :)

* make check disappeared, I assume it doesn't work well?

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #5 from Nelson Marques  2011-12-05 10:28:42 EST ---
Tom,

The most important thing is to serve well and not enter in potential
maintenance nightmares. So here's a question:

- Can we work with two different branches? One for Fedora with the latest
stable release (0.7.0) and one for RHEL with 0.6.1 which has less glitches.

Still on the Fedora realm... here's a few things:

- C# shouldn't be an issue. Your packaging is supreme.
 - haskell, sure.
 - Ruby - I'm not sure if we should provide the gem, as many people seem to
prefer to have the gem built in separate, I've seen this a few times (no clue
why).
 - Java should be ok, at least while RHEL doesn't provide the basic dependency
versioning for ant, Fedora does.

Now regarding perl(Math:BigInt), I followed what the upstream documentation
suggested. I will introduce this change as well.

About %check section, yes, I've removed it.

I'm going to toy a bit more with this and submit hopefully still today a new
spec.

PS: As redundant information, the section I removed with the defines was mainly
used by "Tirpitz", a small script I'm working to take a generic spec and
provide a specific spec according to SUSE/Fedora/RHEL packaging guidelines.
It's still a lot to do, and it's a part of my python learning process. We can
live without it :).

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #6 from Nelson Marques  2011-12-05 10:43:54 EST ---
Extra question:

How well accepted would be something like:

%if 0%{?rhel}
Source0: foobar-1.0.0.tar.gz
%else
Source0: foobar-2.0.0.tar.gz
%endif

Would this be ok considering we would build 0.7.0 for Fedora and 0.6.1 for RHEL
?

NM

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #7 from Tom "spot" Callaway  2011-12-05 12:19:17 EST ---
(In reply to comment #6)
> Extra question:
>
> How well accepted would be something like:
>
> %if 0%{?rhel}
> Source0: foobar-1.0.0.tar.gz
> %else
> Source0: foobar-2.0.0.tar.gz
> %endif
>
> Would this be ok considering we would build 0.7.0 for Fedora and 0.6.1 for RHEL
> ?

I wouldn't do it. The spec will get disgusting, and the changelog won't be sane
anymore.

I'd just do a 0.7.0 build for Fedora and a 0.6.1 build for EPEL. Show me both
specs and I'll sanity check them individually.

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #8 from Nelson Marques  2011-12-07 07:40:49 EST ---
Tom,

Regarding the Fedora stuff here's the deal:

1. C# - upstream doesn't provide a strong name, if I try to install the
assembly with gacutil, it fails. will contact upstream regarding this.

2. JAVA - build is broken against java-openjdk. Either way, which java should
link against?

3. Erlang - Done.

4. Ruby - Should we really provide the ruby gem? Most people I know prefer to
build the gems themselves.

Please advice.

NM

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #9 from Tom "spot" Callaway  2011-12-07 09:34:53 EST ---
I'm not sure why people would prefer to build the ruby gem themselves, but even
if they did, we should still provide it in a package for those who do not.

As to C#, and Java, obviously, if they don't work, you should file bugs
upstream and not enable them at this time. I think as far as which java, I
would say openjdk6 for everything except rawhide, which should use openjdk7.

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #10 from Nelson Marques  2011-12-07 10:58:55 EST ---
(In reply to comment #9)
> I'm not sure why people would prefer to build the ruby gem themselves, but even
> if they did, we should still provide it in a package for those who do not.

Ok, this is already done.
> As to C#, and Java, obviously, if they don't work, you should file bugs
> upstream and not enable them at this time. I think as far as which java, I
> would say openjdk6 for everything except rawhide, which should use openjdk7.

While I'm certain C# issue needs to be fixed by upstream, I'm not sure about
the current JAVA issue which needs a bit more of attention and investigation.

I'll update the request tomorrow once I test properly the JAVA build and
investigate the C# history on JIRA.

I've used the same spec for Fedora and RHEL and so far everything seems to be
working and it's not really that confusing (at least for me).

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #11 from Jeffrey C. Ollie  2011-12-09 15:37:01 EST ---
Created attachment 544692
  --> https://bugzilla.redhat.com/attachment.cgi?id=544692
Build with 0.8.0 on Fedora

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

Jeffrey C. Ollie  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #12 from Jeffrey C. Ollie  2011-12-09 15:39:44 EST ---
Nelson, thanks for taking this on.  I attached a patch to the spec file that
has the changes that I made to get Thrift 0.8.0 to compile on Fedora.  I'm just
getting started with Thrift/Cassandra so I figured I'd start with the latest
available.

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #13 from Nelson Marques  2011-12-09 17:58:15 EST ---
Hi Jeffrey,

I will try to finish this submission this weekend and if possible a few others
I have on the queue (lcm, darwin streaming server and a few perl modules).

I've seen Tom also closed two reviews that will help enable a package I have
for Fedora, so there's quite a few hours I need to fix all of this. Right now
struggling to recover my system as RHEL update from 6.1 to 6.2 has some nefast
effects on my laptop.

NM

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #14 from Nelson Marques  2011-12-12 11:15:53 EST ---
Tom,

Fedora builds are way too problematic and have way too much stuff to fix... I
can't work them on work time because the build requires internet connection for
erlang and java modules, it's dumb enough not to read my proxies, so I will
either need a fix or direct connection to the internet.

Furthermore, if redhat-rpm-config is a BuildRequires, the build just fails on
the tests. Thrift 0.8.0 does seem to require some work to build properly. I
have no interest in delaying Thrift 0.6.1 in EPEL because of this Fedora build.

Anyway we can keep moving with the EPEL build and neglect Fedora submission for
some time so I contact upstream and deal with this properly ?

NM

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #15 from Tom "spot" Callaway  2011-12-12 13:56:48 EST ---
Nelson, I have no problem with you focusing on EPEL here, perhaps Jeff is
interested in maintaining the Fedora side of things? That would be an ideal
situation, as I could review both sides at one ticket, and let you move forward
independently on the EPEL side.

It is concerning that redhat-rpm-config causes the build to fail, as koji will
have it present in the Build Environment even if it is not specified as an
explicit BuildRequires, so I think you may need to fix that now, if it affects
0.6.1.

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #16 from Nelson Marques  2011-12-12 15:13:16 EST ---
Tom,

Regarding 0.8.0 there's a few itches, some are actually new problems:
 * Parallel builds are broken;
 * Unit tests seem fail if redhat-rpm-config is around (needs a bit more of
debugging, and most likely some fixes);

Regarding the modules:
 * Java builds fine and installs fine (against java-1.7.0-openjdk);
 * C# has a open defect[1] on Thrift JIRA since 2009, opened by a Fedora
contributor, but no upstream solution was provided since then;
 * Ruby packaging[2] is a nightmare, I was currently trying to fix this.

I would rather keep myself focused on EPEL and not on Fedora which demands far
more effort.

[1] - https://issues.apache.org/jira/browse/THRIFT-509
[2] -
http://fedoraproject.org/w/index.php?title=Archive:PackagingDrafts/RubyGem_w...

NM

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #17 from Tom "spot" Callaway  2011-12-12 15:19:40 EST ---
(In reply to comment #16)
> I would rather keep myself focused on EPEL and not on Fedora which demands far
> more effort.

Okay. That's fine. It would be nice if someone was interested in working on the
Fedora side in this ticket, but if not, we can work this ticket just for EPEL
and leave Fedora to some other ticket.

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #18 from Nelson Marques  2011-12-13 04:02:07 EST ---
Tom,

If no one takes it, I'll do it, but maybe only after the new year when I will
have a bit more of free time (which is actually the handycap as the package has
a lot of stuff to fix).

It's a nice and challenging package which includes lots of stuff from different
technology and from a packaging point of view it mixes a lot of fun stuff, but
it does require quite a bit more of attention for the tiny details of every
piece of different technology. This is maybe why it is somehow fun, but
complicated and time demanding for the most unexperienced people like myself.

If no one takes it, I'll take a look at it after the new year, meanwhile I'll
just open a few bug reports on thrift JIRA. Meanwhile lets continue with EPEL
one.

NM

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #19 from Nelson Marques  2012-02-29 09:27:20 EST ---
I've started working on a package of thrift 0.8.0 to submit to Fedora
supporting all the dependencies provided by Fedora.

It's a bit later than I expected, but it's not been forgoten.

[Bug 758166] [EPEL] - Review Request -- thrift 0.6.1

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=758166

--- Comment #20 from Tom "spot" Callaway  2012-04-17 11:37:39 EDT ---
I had to make a minor change to the package to get it to build for EPEL-6:

%if 0%{?rhel} <= 5
BuildRequires:  php53-devel
%else
BuildRequires:  php-devel
%endif

Let me know if you want me to just do an EPEL review at this point.