Bug 799810
Summary: | Review Request: python-picloud - PiCloud client-side Library | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Amit Saha <amitsaha.in> | ||||
Component: | Package Review | Assignee: | Christoph Wickert <christoph.wickert> | ||||
Status: | CLOSED WONTFIX | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | unspecified | ||||||
Version: | rawhide | CC: | asaha, christoph.wickert, liling, mcepl, package-review, volker27 | ||||
Target Milestone: | --- | Flags: | christoph.wickert:
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 20:13:22 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Attachments: |
|
Description
Amit Saha
2012-03-05 06:03:27 UTC
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 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. 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. 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 (In reply to comment #4) You're getting me wrong here. I'm referring to _sysconfdir and _datadir. (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 (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! Please always bump the release when you change something and post the links. That's easier for reviewers. (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. 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> 2.4.2 should be * Fri Mar 9 2012 Amit Saha <amitksaha> 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. (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> 2.4.2 > > should be > > * Fri Mar 9 2012 Amit Saha <amitksaha> 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 New upstream release. Updated files: SPEC: http://amitksaha.fedorapeople.org/contribs/picloud_packaging/python-picloud.spec SRPM: http://amitksaha.fedorapeople.org/contribs/picloud_packaging/python-picloud-2.4.4-1.fc16.src.rpm New upstream release. Updated files: SPEC: https://github.com/amitsaha/rpm_spec/raw/master/python-picloud.spec SRPM: https://github.com/amitsaha/rpm_spec/blob/master/python-picloud-2.5.6-1.fc17.src.rpm Amit, have you searched for a sponsor yet? 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/ |