Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=879749 Michael Scherer <misc@xxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends On| |879752 --- Comment #4 from Michael Scherer <misc@xxxxxxxx> --- 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. -- You are receiving this mail because: You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review