This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 495357 - Review Request: python-twill - Simple scripting language for Web browsing
Review Request: python-twill - Simple scripting language for Web browsing
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Till Maas
Fedora Extras Quality Assurance
:
: 509995 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-12 06:18 EDT by Matthias Saou
Modified: 2009-12-09 12:55 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-16 16:52:48 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
opensource: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Matthias Saou 2009-04-12 06:18:01 EDT
Spec URL: http://thias.fedorapeople.org/review/python-twill/python-twill.spec
SRPM URL: http://thias.fedorapeople.org/review/python-twill/python-twill-0.9-1.src.rpm
Description:
twill is a simple language that allows users to browse the Web from a
command-line interface. With twill, you can navigate through Web sites that
use forms, cookies, and most standard Web features.

Note: The original submission (bug #253355), so I'm re-submitting the latest cleaned up package I had ready.
Comment 1 Till Maas 2009-04-12 07:50:55 EDT
Just some quick observations:

The pyver macro seems not to be used, therefore it should be probably skipped:

%{!?pyver: %define pyver %(%{__python} -c "import sys ; print sys.version[:3]")}

Also this should use at least %global instead of %define:

%{!?python_sitelib: %define python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print get_python_lib()")}

Reference:
https://fedoraproject.org/wiki/Packaging:Python#System_Architecture

Also the patch is not commented.

Reference:
https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment
Comment 2 Matthias Saou 2009-04-12 13:26:38 EDT
I've removed the unused pyver, used %global instead of %define.
The patch did have a comment, but above the %patch0 line instead of the Patch0: one. I've moved it. The logic here is simple : Upstream bundles a bunch of 3rd party python code to make things easier for end users, but we happen to already have them available as packages, so what the patch does is use the package provided code instead of the "internal" forked code. Definitely not worth reporting upstream.

I've searched the wiki for the guideline about this, as I'm pretty sure there is one about always using a system wide library instead of a project-provided local version, but I've been unable to find it.

Thanks for your comments, I've updated the spec and package (didn't bother bumping the release just for that, though).
Comment 3 Till Maas 2009-04-13 08:53:26 EDT
(In reply to comment #2)

> I've searched the wiki for the guideline about this, as I'm pretty sure there
> is one about always using a system wide library instead of a project-provided
> local version, but I've been unable to find it.

https://fedoraproject.org/wiki/Packaging/Guidelines#Duplication_of_system_libraries
 
> Thanks for your comments, I've updated the spec and package (didn't bother
> bumping the release just for that, though).  

Imho it is better to always bump the release, because it reduces confusion about different source rpm files with the same filename.
Comment 4 Till Maas 2009-04-13 11:26:51 EDT
[GOOD ENOUGH] rpmlint output:
python-twill.noarch: E: non-standard-executable-perm /usr/bin/twill-fork 0775
It seems that the umask of the user building the rpm affects the permissions
of the files inside the rpm. This is not nice imho.

[OK] Spec in %{name}.spec format

[OK] license allowed: MIT
[OK] license matches shortname in License: tag
[OK] license in tarball and included in %doc: LICENSE.txt

[OK] package is code or permissive content:

{OK} patches sent to upstream and commented

[OK] Source0 is a working URL
{N/A} Sourceforge URL is Source0: http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz
<GOOD ENOUGH> SourceX / PatchY prefixed with %{name}
It is nicer to have future patches in the form:
%{name}-%{version}-$SUFFIX.patch
and to use -b .$SUFFIX in the according %patchX command to make rediffing
easier.

[OK] Source0 matches Upstream:
c362307616696f4838e9456c42b70fdc  twill-0.9.tar.gz

[OK] Package builds on all platforms:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1294675
[N/A] ExcludeArch bugs are filed and commented:
[OK] BuildRequires are complete (mock builds)
(OK) No file dependencies outside of /etc /bin /sbin /usr/bin /usr/sbin 

[N/A] %find_lang used for locales

[N/A] Every (sub)package containing libraries runs ldconfig
[N/A] .h (header) files are in -devel subpackage
[N/A] .a (static libraries) are in -static subpackage
[N/A] contains .pc (pkgconfig) files and has Requires: pkgconfig
(N/A) .pc files are in -devel subpackage
[N/A] contains .so.X(.Y) files and .so is in -devel
[N/A] -devel subpackage has Requires: %{name} = %{version}-%{release}
[N/A] .la files (libtool) are not included


[N/A] Has GUI and includes %{name}.desktop
[N/A] Follows desktop entry spec
[N/A] Valid .desktop Name
[N/A] Valid .desktop GenericName
[N/A] Valid .desktop Categories
[N/A] Valid .desktop StartupNotify
[N/A] .desktop file installed with desktop-file-install in %install

[OK] Prefix: /usr not used (not relocatable)

[OK] Owns all created directories
[OK] no duplicates in %files
[OK] %defattr(-,root,root,-) is in every %files section
[OK] Does not own files or dirs from other packages
[OK] included filenames are in UTF-8

[OK] %clean is rm -rf %{buildroot} or $RPM_BUILD_ROOT 
[OK] %install starts with rm -rf %{buildroot} or $RPM_BUILD_ROOT 

[OK] Consistent macro usage

[OK] large documentation is -doc subpackage
[OK] %doc does not affect runtime

{OK} no pre-built binaries (.a, .so*, executable)

{OK} well known BuildRoot
%{_tmppath}/%{name}-%{version}-%{release}-root
{OK} PreReq not used
{N/A} RPM_OPT_FLAGS honoured
{N/A} Useful debuginfo generated
{OK} no duplication of system libraries
{N/A} no rpath
{N/A} Timestamps preserved with cp and install
{N/A} Uses parallel make (%{?_smp_mflags})
{OK} Requires(pre,post) style notation not used
{OK} only writes to tmp /var/tmp $TMPDIR %{_tmppath} %{_builddir} (and %{buildroot} on %install and %clean)
{OK} no Conflicts
{OK} nothing installed in /srv
{OK} Changelog in allowed format
{OK} does not use Scriptlets
<OK> Architecture independent packages have: BuildArch: noarch
<OK> Sane Provides: and Requires:

{OK} Follows Naming Guidelines
Python 

{OK} Has BuildRequires: python - via BR: python-setuptools or python-devel
{OK} Defines and uses %{python_sitelib}:
%{!?python_sitelib: %global python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print get_python_lib()")}
{N/A} Defines and uses %{python_sitearch}:
%{!?python_sitearch: %global python_sitearch %(%{__python} -c "from %distutils.sysconfig import get_python_lib; print get_python_lib(1)")}

{GOOD ENOUGH} Has BuildRequires: python-setuptools-devel
Seems not to be needed. Only easy_install seems to be in the -devel package. Not sure, why the Guidelines say it should be there.

[OK] Python eggs must be built from source. They cannot simply drop an egg from upstream into the proper directory.
[OK] Python eggs must not download any dependencies during the build process.
[OK] If egg-info files are generated by the modules build scripts they must be included in the package.
[N/A] When building a compat package, it must install using easy_install -m so it won't conflict with the main package.
[N/A] When building multiple versions (for a compat package) one of the packages must contain a default version that is usable via "import MODULE" with no prior setup.
(OK) A package which is used by another package via an egg interface should provide egg info. 

{NOT OK} Egg install:
%install
%{__python} setup.py install --skip-build --root $RPM_BUILD_ROOT 
The "--single-version-externally-managed" is not needed anymore:
https://fedoraproject.org/wiki/Packaging/Python/Eggs#Providing_Eggs_using_Setuptools

There are only minor issues or issues that do not affect rpms building within the Fedora buildsystem:

Please change the build to not let the umask of the user building the rpm affect the final rpm. I am not sure, whether this is a default bad beheaviour of the  python build system or not.

Please consider adding -b .fork to the %patch0 to make rediffing easier and plase use the package name in the patch in the future.


Please remove the uneeded argument in %install before importing this package into Fedora.

APPROVED
Comment 5 Matthias Saou 2009-04-14 04:42:34 EDT
Thanks a lot for the review!

* Tue Apr 14 2009 Matthias Saou <http://freshrpms.net/> 0.9-2
- Add -b .noforks to the patch0 line.
- Remove no longer needed --single-version-externally-managed option.

About the file mode, I've found nothing special after a quick search in the sources, and saw that the koji build has "changing mode of /builddir/build/BUILDROOT/python-twill-0.9-1.fc11.noarch/usr/bin/twill-fork to 755", so I think it's the default python build/install.
Comment 6 Matthias Saou 2009-04-14 04:42:53 EDT
New Package CVS Request
=======================
Package Name: python-twill
Short Description: Simple scripting language for Web browsing
Owners: thias
Branches: F-9 F-10
InitialCC:
Comment 7 Kevin Fenzi 2009-04-14 12:16:04 EDT
cvs done.
Comment 8 Matthias Saou 2009-04-16 16:52:48 EDT
Thanks Till and Kevin. All packages built and pushed to all branches.
Comment 9 Matthias Saou 2009-12-08 14:11:01 EST
Package Change Request
======================
Package Name: python-twill
New Branches: EL5
Owners: thias
Comment 10 Matthias Saou 2009-12-08 14:12:39 EST
*** Bug 509995 has been marked as a duplicate of this bug. ***
Comment 11 Kevin Fenzi 2009-12-09 12:55:08 EST
cvs done

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