Bug 1126100 - Review Request: disco - Erlang/Python Lightweight Map Reduce Framework
Review Request: disco - Erlang/Python Lightweight Map Reduce Framework
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: José Matos
Fedora Extras Quality Assurance
Depends On:
  Show dependency treegraph
Reported: 2014-08-01 17:13 EDT by Tait Clarridge
Modified: 2015-08-15 09:25 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)

  None (edit)
Description Tait Clarridge 2014-08-01 17:13:44 EDT
Spec URL: https://github.com/oldmantaiter/fedpkg/raw/master/disco/disco.spec
SRPM URL: https://github.com/oldmantaiter/fedpkg/raw/master/disco/disco-0.5.3-1.fc20.src.rpm
Description: Disco is a lightweight, open-source framework for distributed computing based on the MapReduce paradigm.
Fedora Account System Username: oldmantaiter
Comment 1 Tait Clarridge 2014-08-01 17:21:02 EDT

This is my first (hopefully of many) packages, so I am looking for a sponsor.
I have a scratch build completed for both epel7 and f20:



Disco website: http://discoproject.org

Comment 2 Tait Clarridge 2014-08-02 08:57:00 EDT
Rewriting part of the spec file, will update the ticket when I have the new scratch builds.
Comment 3 Tait Clarridge 2014-08-02 13:03:18 EDT
Updated spec and SRPM are at the original URLs.

Scratch builds:


Comment 4 Christopher Meng 2014-08-04 05:13:12 EDT
1. Drop these 2:

BuildRoot: %{_tmppath}/disco-%{version}-root
Vendor: Disco Authors

Ref: https://fedoraproject.org/wiki/Packaging:Guidelines#Tags

2. All Group: could be dropped as well, as Fedora doesn't need them anymore.

3. Drop obsoleted stuffs:

BuildRoot: %{_tmppath}/disco-%{version}-root



4. %{__make} -> make
%{_prefix}/bin -> %{_bindir}
%{_prefix}/share -> %{_datadir}
== -> =
%setup -n disco -> %setup -q -n disco

5. cp bin/disco bin/ddfs $RPM_BUILD_ROOT/%{_prefix}/bin

Use install -pm755 to set perms instead of %attr(0755,root,root), the latter one has been abused too much.

6. DISCO_VERSION=%{version}  %{__python} setup.py install -O1 --root=$RPM_BUILD_ROOT


Please set PYTHON to %{__python2} explicitly.

7. %dir %{_prefix}/var/disco
%dir %{_prefix}/lib/disco

Violates FHS.

8. Add %global debug_package %{nil} to the erlang package.

9. Drop the dot:

Summary:  An open-source mapreduce framework.

Not completed, but you should fix them. Basically, not good.
Comment 5 Tait Clarridge 2014-08-04 08:20:41 EDT
Hi Christopher,

I will read over the Packaging Guidelines carefully, I appreciate you taking the time to give me some pointers and corrections and will update the spec accordingly.

Comment 6 José Matos 2014-08-05 10:48:49 EDT
Hi Tait,
  note that rpm spec files have become a lot saner with time and some of the changes reflect precisely that. An example, why should the BuildRoot be defined in the spec files?

In another case (6) it all comes to the fact that soon we will change our default version of python from 2 to 3 and thus it is better to be explicit and declare what is the version really used. Before python 3 this was not an issue.

In the case of the summary the convention is not to end the sentence with a dot.

So there are reasons for all the changes required and the documentation is thorough, feel free to peruse it. :-)

One advice it to try to build your packages using fedora-review, it is just a program and it does not replace human reasoning but it is a nice starting point. At the same time try to review other submissions to increase your fluency in rpm spec.

I am a sponsor and as you can read the informal review of others request is a major point for approval.
Comment 7 Tait Clarridge 2014-08-05 11:03:03 EDT
Hi Jose,

I completely understand the reasoning for the peer review process and how important it is to maintain standards. I really appreciate both you and Christopher taking the time to assist me and point out where I need work. Always willing to learn, and I have been reading the guidelines while going through the spec I changed line by line. I'll also take a look at other submissions (maybe for packages I have done "in-house") and learn from those too.

Comment 8 José Matos 2014-08-05 11:42:29 EDT
The purpose of my message was supposed to be lighthearted. :-)

And somehow it missed that tone as I wrote.

What you have is a nice starting point and you can count on us to help. If it sounded other way than that it was my fault. :-)
Comment 9 Tait Clarridge 2014-08-05 11:51:42 EDT
Don't worry, I read all the messages so far as lighthearted and friendly assistance :)

Mine were too, just emphasizing the gratefulness for the help and pointers because of the importance of packaging correctly. Sorry if mine came off as combative, they were not meant to be!

As for some packaging questions:

For the disco dependencies, they use specific versions of erlang modules and thus were put into a special directory. Would it be OK if I follow the erlang package in putting disco code into /usr/lib64/erlang/lib/disco-%{version} where normal packages have the directory structure of /{ebin,include,priv,src} but disco would be /master/{deps,ebin}?
Comment 10 Tait Clarridge 2014-08-05 12:57:44 EDT
Found http://fedoraproject.org/wiki/User:Peter/Erlang_Packaging_Guidelines. Will abide by that for the Erlang side of things.
Comment 11 Tait Clarridge 2014-08-05 23:03:36 EDT
Ran fedora-review (amazing tool by the way!) and went through as much as I could see as being an issue. I also updated my dev box to rawhide.

Checking: disco-0.5.3-1.fc22.x86_64.rpm
disco.x86_64: E: no-binary
disco.x86_64: W: only-non-binary-in-usr-lib
disco-master.x86_64: W: no-documentation
disco.src: W: invalid-url Source0: disco.tar.gz
5 packages and 0 specfiles checked; 1 errors, 3 warnings.

* W: invalid-url Source0: disco.tar.gz - There is a comment in the spec file regarding this warning, disco requires the git repository to set the version for the erlang package from the gitlog.

* E: no-binary - I've seen in some other erlang related packages, no-binary can be safely ignored as it is arch dependant for %{_libdir} (see bug 906473 comment 2). 

Also, in the Erlang guidelines it was asked that any erlang package have erlang-%{name}, but in this case I chose to leave it as disco as it is more of a standalone application and not one people would include in their own apps.

I have not tested disco running on anything other than x86_64 and i686, should these arches be the only ones specified? I can try emulating arm as well and seeing if they run as expected as well.

Spec URL: https://github.com/oldmantaiter/fedpkg/raw/master/disco/disco.spec
SRPM URL: https://github.com/oldmantaiter/fedpkg/raw/master/disco/disco-0.5.3-1.fc22.src.rpm

Koji scratch build for rawhide:

Comment 12 José Matos 2015-07-31 08:33:29 EDT
Oops. Time really flies... :-)

Are you still interested in being sponsored?


If so I will help you to go through all the stages to become a packager.
Comment 13 Tait Clarridge 2015-08-05 08:03:23 EDT
I am definitely still interested, but I am on vacation until August 17th so I will respond when I'm back and can take the time to properly go through the steps.

Thanks for offering to sponsor me, I am looking forward to learning with your guidance and getting to a point where I am able to contribute.
Comment 14 José Matos 2015-08-15 09:25:40 EDT
Sure. :-)
I will be away next week so take your time. :-)

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