Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=879749
Bug ID: 879749 Summary: Review Request: xs-activation - OLPC XS Activation Server Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: unspecified Reporter: aadavis1@learn.senecac.on.ca
Spec URL: http://matrix.senecac.on.ca/~aadavis1/packaging/xs-activation.spec SRPM URL: http://matrix.senecac.on.ca/~aadavis1/packaging/xs-activation-0.3.14.gd2a372... Description: Hi! I just finished packaging up xs-activation, and I would appreciate a review so that I can get it into fedora extras.
Fedora Account System Username:aadavis1
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=879749
Michael Scherer misc@zarb.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |misc@zarb.org Blocks| |177841 (FE-NEEDSPONSOR)
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=879749
Michael Scherer misc@zarb.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Resolution|--- |DUPLICATE Last Closed| |2012-11-24 08:43:06
--- Comment #1 from Michael Scherer misc@zarb.org --- Hi alex,
this package was already submitted and refuse, please look at https://bugzilla.redhat.com/show_bug.cgi?id=879568 for the reason ( and a few notes on the previously exposed spec )
*** This bug has been marked as a duplicate of bug 879568 ***
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=879749
Michael Scherer misc@zarb.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|CLOSED |ASSIGNED Resolution|DUPLICATE |--- Keywords| |Reopened
--- Comment #2 from Michael Scherer misc@zarb.org --- Oups, wrong bug, sorry
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=879749
Michael Scherer misc@zarb.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |NEW
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=879749
--- Comment #3 from Michael Scherer misc@zarb.org --- So,
I assume you do not have yet a sponsor, I can review and help you, but you will need to find one ( following https://fedoraproject.org/wiki/Package_Review_Process ) to sponsor if your task is to have them in Fedora. I applied for sponsorship 1 hour ago, but I am not guaranteed to become one, so better try to find one ( as i said to your comrade, I would suggest to get in touch with ctyler ).
In the mean time, i will review this package.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=879749
Michael Scherer misc@zarb.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends On| |879752
--- Comment #4 from Michael Scherer misc@zarb.org --- A few notes as part of the review :
1) Packager tag should not be used https://fedoraproject.org/wiki/Packaging:Guidelines#Tags
2) I do not think %post should be kept, as people may not read it, and that it doesn't help much. I think there is even a policy to say that %post should be silent.
3) %install echo "hello" #rm -rf $RPM_BUILD_ROOT pwd ls make DESTDIR=$RPM_BUILD_ROOT PYTHON_SITELIB=%{python_sitelib} install
no need for echo, pwd, ls, as this is likely just for debugging.
4) having /library is forbidden in Fedora : https://fedoraproject.org/wiki/Packaging:Guidelines#Filesystem_Layout
We cannot create arbitrary top level directory. So you should see with upstream to change this.
5) the changelog entry should be more descriptive https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs ( ie, explain what you changed before the previous version and this one
6) BuildArch: x86_64
Why limit to x86_64 ?
7) THis one is subtle. %{_sysconfdir}/sysconfig/olpc-scripts/setup.d/*
If you install xs-activation, and remove it, as the directory /etc/sysconfig/olpc-scripts/ is not listed in %files, it would not be removed, and so this would be a leftover. We try to avoid that. See https://fedoraproject.org/wiki/Packaging:UnownedDirectories for details
8) BuildRequires: python-devel
you need to explin if this is python2 or python3 https://fedoraproject.org/wiki/Packaging:Python#BuildRequires ( otherwise, this may break the day we switch to python3, so we try to be proactive and prevent the issue before it happens )
9) Requires: bash Requires: python
Bash is preinstalled, and I think python will be automatically detected ( ie, rpm will add the requires by itself )
10) Requires: usbmount
usbmount is not in Fedora, so the package need to be added.
11) %{!?python_sitelib: %global python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print get_python_lib()")}
not sure if that's needed anymore, since all supported Fedora should already have the macro defined
https://fedoraproject.org/wiki/Packaging:Python#Macros
12) the description is rather terse, and could IMHO be improved.
13) I think a better url would be http://wiki.laptop.org/go/XS-activation
Do not hesitate to contact me ( either misc, on irc.freenode.net ), or ask question in this bug if there is something unclear.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=879749
--- Comment #5 from Alex aadavis1@learn.senecac.on.ca --- (In reply to comment #4)
A few notes as part of the review :
- Packager tag should not be used
https://fedoraproject.org/wiki/Packaging:Guidelines#Tags
- I do not think %post should be kept, as people may not read it, and that
it doesn't help much. I think there is even a policy to say that %post should be silent.
%install echo "hello" #rm -rf $RPM_BUILD_ROOT pwd ls make DESTDIR=$RPM_BUILD_ROOT PYTHON_SITELIB=%{python_sitelib} install
no need for echo, pwd, ls, as this is likely just for debugging.
- having /library is forbidden in Fedora :
https://fedoraproject.org/wiki/Packaging:Guidelines#Filesystem_Layout
We cannot create arbitrary top level directory. So you should see with upstream to change this.
- the changelog entry should be more descriptive
https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs ( ie, explain what you changed before the previous version and this one
- BuildArch: x86_64
Why limit to x86_64 ?
- THis one is subtle.
%{_sysconfdir}/sysconfig/olpc-scripts/setup.d/*
If you install xs-activation, and remove it, as the directory /etc/sysconfig/olpc-scripts/ is not listed in %files, it would not be removed, and so this would be a leftover. We try to avoid that. See https://fedoraproject.org/wiki/Packaging:UnownedDirectories for details
BuildRequires: python-devel
you need to explin if this is python2 or python3 https://fedoraproject.org/wiki/Packaging:Python#BuildRequires ( otherwise, this may break the day we switch to python3, so we try to be proactive and prevent the issue before it happens )
Requires: bash Requires: python
Bash is preinstalled, and I think python will be automatically detected ( ie, rpm will add the requires by itself )
Requires: usbmount
usbmount is not in Fedora, so the package need to be added.
%{!?python_sitelib: %global python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print get_python_lib()")}
not sure if that's needed anymore, since all supported Fedora should already have the macro defined
https://fedoraproject.org/wiki/Packaging:Python#Macros
the description is rather terse, and could IMHO be improved.
I think a better url would be http://wiki.laptop.org/go/XS-activation
Do not hesitate to contact me ( either misc, on irc.freenode.net ), or ask question in this bug if there is something unclear.
thanks.
for the x86_64 I use this because I was to do a x86_64 or i386 build on the package. I thought if noarch build was removed I will get an x86_64.
I made changes to the correction should I resubmit the review?
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=879749
--- Comment #6 from Michael Scherer misc@zarb.org --- For package, if you want to do x86_64, you build on x86_64 host, and the same for i386.
Once you have made correction, just give the url to the new version of the spec file and srpm.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=879749
--- Comment #7 from Alex aadavis1@learn.senecac.on.ca --- Spec URL: http://matrix.senecac.on.ca/~aadavis1/packaging/xs-activation.spec SRPM URL: http://matrix.senecac.on.ca/~aadavis1/packaging/xs-activation-0.3.14.gd2a372... Description: Hi! I just finished packaging up xs-activation, and I would appreciate a review so that I can get it into fedora extras.
correction have been made, I just comment (#) the places where a few things are not needed and update the changelog.
Fedora Account System Username:aadavis1
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=879749
--- Comment #8 from Alex aadavis1@learn.senecac.on.ca --- hey I think uploading this in the fedora review will mark it as a duplicate so I post the links above. it the same source but file is uploaded.
just waiting for a sponsor
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=879749
--- Comment #9 from Alex aadavis1@learn.senecac.on.ca --- hey I think uploading this in the fedora review will mark it as a duplicate so I post the links above. it the same source but file is uploaded.
just waiting for a sponsor
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=879749
--- Comment #10 from Alex aadavis1@learn.senecac.on.ca --- Spec URL: http://matrix.senecac.on.ca/~aadavis1/packaging/xs-activation.spec SRPM URL: http://matrix.senecac.on.ca/~aadavis1/packaging/xs-activation-0.3.14.gd2a372... Description: Hi, I have rebuild this package to meet Fedora Packaging Guideline.I would appreciate your feedback and review.
Fedora Account System Username: aadavis1
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=879749
Alex aadavis1@learn.senecac.on.ca changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(misc@zarb.org)
package-review@lists.fedoraproject.org