Bug 799810

Summary: Review Request: python-picloud - PiCloud client-side Library
Product: [Fedora] Fedora Reporter: Amit Saha <amitsaha.in>
Component: Package ReviewAssignee: Christoph Wickert <cwickert>
Status: CLOSED WONTFIX QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: asaha, cwickert, liling, package-review, volker27
Target Milestone: ---Flags: cwickert: fedora‑review?
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-11-21 15:13:22 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Attachments:
Description Flags
build log from the koji scratch build none

Description Amit Saha 2012-03-05 01:03:27 EST
Spec URL: http://amitksaha.fedorapeople.org/contribs/picloud_packaging/python-picloud.spec
SRPM URL: http://amitksaha.fedorapeople.org/contribs/picloud_packaging/python-picloud-2.4.2-1.fc16.src.rpm
Description: PiCloud is a cloud-computing platform that integrates into 
the Python Programming Language. This is the package for PiCloud client-side library.

This is my first package and I am in need of a sponsor. 

I have tried to ensure that the package conforms to the guidelines, but that may not be the case. Thanks!
Comment 1 Volker Fröhlich 2012-03-05 02:08:00 EST
Just a few comments:

- Harmonize on either using $RPM_BUILD_ROOT or %{buildroot}
- Use the _mandir and _sysconfdir macro in the install section and preserve timestamps for the files you install
- BR python-devel, see http://fedoraproject.org/wiki/Packaging:Python#BuildRequires
- Package fails to build for me (Rawhide, i386) -- probably for the reason above
- Since EL 5 has Python 2.4, you're package probably won't work. In that case, you should remove the rm in the install section and the buildroot definition -- http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#EL5
Comment 2 Amit Saha 2012-03-05 02:29:10 EST
Thanks for your comments:

(In reply to comment #1)
> Just a few comments:
> 
> - Harmonize on either using $RPM_BUILD_ROOT or %{buildroot}
> - Use the _mandir and _sysconfdir macro in the install section and preserve
> timestamps for the files you install
> - BR python-devel, see
> http://fedoraproject.org/wiki/Packaging:Python#BuildRequires
> - Package fails to build for me (Rawhide, i386) -- probably for the reason
> above
> - Since EL 5 has Python 2.4, you're package probably won't work. In that case,
> you should remove the rm in the install section and the buildroot definition --
> http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#EL5


Can you please check the SPEC file now: http://amitksaha.fedorapeople.org/contribs/picloud_packaging/python-picloud.spec ?

Cheers.
Comment 3 Volker Fröhlich 2012-03-06 09:09:54 EST
It's not LGPLv2 but LGPLv2+. APL 2.0 and LGPLv2 are not compatible, as far as I can see, so you should mention both, as you do. But the three clause BSD without advertising that src/util/cronexpr.py has, is compatible with GPL. So i think the overall license should be LGPLv2+ and ASL 2.0.

Use the macros with mkdir as well!

Remove the Requires for Python at all.

What is it about this comment:
# These packages are not require for python >=2.6 but required for python = 2.5

I'm getting two warnings:

#Warning: Could not install bash completion scripts due to [Errno 13] Permission denied: '/etc/bash_completion.d/picloud'
#Warning: Could not install bash completion scripts due to [Errno 13] Permission denied: '/usr/share/man/man1/picloud.1'

The last line in the files section should be something like:

%config(noreplace) %{_sysconfdir}/bash_completion.d/*

Otherwise you'll own /etc and /etc/bash_completion. Why do you feel this should be "(noreplace)"? It is not something users usually customize, or is it?

The package still doesn't build in Mock.
Comment 4 Matěj Cepl 2012-03-06 17:44:57 EST
Created attachment 568090 [details]
build log from the koji scratch build

(In reply to comment #3)
> Use the macros with mkdir as well!

Please. None of these silly macros ... see the thread on fedora-devel.

> What is it about this comment:
> # These packages are not require for python >=2.6 but required for python = 2.5

yes, get rid of these ... there is no python 2.5 anywhere in the Fedora/RHEL world.

> The package still doesn't build in Mock.

Neither does in koji http://koji.fedoraproject.org/koji/taskinfo?taskID=3861037
Comment 5 Volker Fröhlich 2012-03-06 17:57:28 EST
(In reply to comment #4)

You're getting me wrong here. I'm referring to _sysconfdir and _datadir.
Comment 6 Amit Saha 2012-03-06 19:48:32 EST
(In reply to comment #4)
> Created attachment 568090 [details]
> build log from the koji scratch build
> 
> (In reply to comment #3)
> > Use the macros with mkdir as well!
> 
> Please. None of these silly macros ... see the thread on fedora-devel.
> 
> > What is it about this comment:
> > # These packages are not require for python >=2.6 but required for python = 2.5
> 
> yes, get rid of these ... there is no python 2.5 anywhere in the Fedora/RHEL
> world.
> 
> > The package still doesn't build in Mock.

This is probably a hack, but adding this line in the %install section allows mock to pass successfully:

mkdir -p $RPM_BUILD_ROOT/%{_bindir}/

I have updated the spec file and SRPMS with the other comments. Please take a look.


> 
> Neither does in koji http://koji.fedoraproject.org/koji/taskinfo?taskID=3861037
Comment 7 Amit Saha 2012-03-09 09:43:16 EST
(In reply to comment #3)
> It's not LGPLv2 but LGPLv2+. APL 2.0 and LGPLv2 are not compatible, as far as I
> can see, so you should mention both, as you do. But the three clause BSD
> without advertising that src/util/cronexpr.py has, is compatible with GPL. So i
> think the overall license should be LGPLv2+ and ASL 2.0.
> 
> Use the macros with mkdir as well!
> 
> Remove the Requires for Python at all.
> 
> What is it about this comment:
> # These packages are not require for python >=2.6 but required for python = 2.5
> 
> I'm getting two warnings:
> 
> #Warning: Could not install bash completion scripts due to [Errno 13]
> Permission denied: '/etc/bash_completion.d/picloud'
> #Warning: Could not install bash completion scripts due to [Errno 13]
> Permission denied: '/usr/share/man/man1/picloud.1'
> 
> The last line in the files section should be something like:
> 
> %config(noreplace) %{_sysconfdir}/bash_completion.d/*
> 
> Otherwise you'll own /etc and /etc/bash_completion. Why do you feel this should
> be "(noreplace)"? It is not something users usually customize, or is it?
> 
> The package still doesn't build in Mock.

Can you please take a look at the current SPEC and SRPM? Thanks for your comments!
Comment 8 Volker Fröhlich 2012-03-09 11:07:15 EST
Please always bump the release when you change something and post the links. That's easier for reviewers.
Comment 9 Amit Saha 2012-03-09 18:48:20 EST
(In reply to comment #8)
> Please always bump the release when you change something and post the links.
> That's easier for reviewers.

Thanks! Please find the revised files:

SPEC: http://amitksaha.fedorapeople.org/contribs/picloud_packaging/python-picloud.spec

SRPM: http://amitksaha.fedorapeople.org/contribs/picloud_packaging/python-picloud-2.4.2-2.fc16.src.rpm

Shall look forward to your comments.
Comment 10 Volker Fröhlich 2012-03-11 13:22:01 EDT
Please look at these examples:

http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs

Your format is the one without the dash, but you're missing the release number.

* Fri Mar 9 2012 Amit Saha <amitksaha@fedoraproject.org> 2.4.2

should be

* Fri Mar 9 2012 Amit Saha <amitksaha@fedoraproject.org> 2.4.2-2

%{_bindir}/ must be %{_bindir}/*, otherwise you're trying to own /usr/bin.

You could also consider to patch setup.py. That'd get you rid of the error message while installing and you don't have to install manpage and bash completion file on your own.

On second thoughts: The bash completion should probably not be considered configuration at all. I looked through a couple of bash completion files (yum, mock, git, bash-completion) and none of them labels it as configuration. I guess you should do the same.

rpm -qf /etc/bash_completion.d/mock.bash
rpm -qc mock
...

If you incorporate these changes, the package is pretty fine from my point of view. Nevertheslles I can't take the review, because you need a sponsor.

For the reviewer: The package now builds in Mock.
Comment 11 Amit Saha 2012-03-13 22:08:41 EDT
(In reply to comment #10)
> Please look at these examples:
> 
> http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
> 
> Your format is the one without the dash, but you're missing the release number.
> 
> * Fri Mar 9 2012 Amit Saha <amitksaha@fedoraproject.org> 2.4.2
> 
> should be
> 
> * Fri Mar 9 2012 Amit Saha <amitksaha@fedoraproject.org> 2.4.2-2
> 
> %{_bindir}/ must be %{_bindir}/*, otherwise you're trying to own /usr/bin.
> 
> You could also consider to patch setup.py. That'd get you rid of the error
> message while installing and you don't have to install manpage and bash
> completion file on your own.
> 
> On second thoughts: The bash completion should probably not be considered
> configuration at all. I looked through a couple of bash completion files (yum,
> mock, git, bash-completion) and none of them labels it as configuration. I
> guess you should do the same.
> 
> rpm -qf /etc/bash_completion.d/mock.bash
> rpm -qc mock
> ...
> 
> If you incorporate these changes, the package is pretty fine from my point of
> view. Nevertheslles I can't take the review, because you need a sponsor.
> 
> For the reviewer: The package now builds in Mock.

Many thanks for the comments. I have incorporated these changes, and checked in Mock. It seems to be fine. Updated files:

SPEC file: http://amitksaha.fedorapeople.org/contribs/picloud_packaging/python-picloud.spec

SRPM: http://amitksaha.fedorapeople.org/contribs/picloud_packaging/python-picloud-2.4.2-3.fc16.src.rpm
Comment 14 Volker Fröhlich 2013-04-26 11:13:11 EDT
Amit, have you searched for a sponsor yet?
Comment 15 Amit Saha 2013-11-21 15:13:22 EST
Closing this for now, since PiCloud is shutting down [1], and I am not sure if at all I will ever work on a client for the new service that PiCloud is going to become.

[1] http://blog.picloud.com/2013/11/17/picloud-has-joined-dropbox/