Bug 1128100 - Review Request: python-markups - A wrapper around various text markups
Summary: Review Request: python-markups - A wrapper around various text markups
Keywords:
Status: CLOSED DUPLICATE of bug 1169493
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 1119007 (view as bug list)
Depends On:
Blocks: 1128101
TreeView+ depends on / blocked
 
Reported: 2014-08-08 10:00 UTC by Nikos Roussos
Modified: 2016-01-16 23:16 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-01-16 23:16:16 UTC


Attachments (Terms of Use)

Description Nikos Roussos 2014-08-08 10:00:12 UTC
Spec URL: https://comzeradd.fedorapeople.org/specs/python-markups.spec
SRPM URL: https://comzeradd.fedorapeople.org/srpms/python-markups-0.5.0-1.fc20.src.rpm
Description: A wrapper around various text markups
Fedora Account System Username: comzeradd

Comment 1 Volker Fröhlich 2014-08-08 15:58:17 UTC
The tests are failing in at least F20 and Rawhide:

======================================================================
FAIL: test_api (tests.test_public_api.APITest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/makerpm/rpmbuild/BUILD/Markups-0.5.0/tests/test_public_api.py", line 18, in test_api
    self.assertTrue(isinstance(markup, markups.MarkdownMarkup))
AssertionError: False is not true

Remove the version constraints from the BRs. They are fulfilled in every release of Fedora.

Use %global instead of %define.

Use a wildcard for the Python version for the egg-info in the files section, otherwise you'd have to adapt it on branches and new releases!

Comment 2 Volker Fröhlich 2014-08-08 16:10:18 UTC
Please delete the egg-info directory in the prep section. It should only be included if it's generated by the build script.

Comment 3 Nikos Roussos 2014-08-09 09:01:01 UTC
Thanks for looking into this. I think I fixed most of the issues.

SPEC: https://comzeradd.fedorapeople.org/specs/python-markups.spec
SRPM: https://comzeradd.fedorapeople.org/srpms/python-markups-0.5.0-2.fc20.src.rpm

Comment 4 Christopher Meng 2014-08-09 10:08:31 UTC
I packaged it month ago. 

IMO this package should be named python-Markups.

Comment 5 Nikos Roussos 2014-08-09 10:48:55 UTC
I just followed what seems to be the current trend at python packages, to be all lowercase. For instance Markdown is packaged as python-markdown.

Comment 6 Eduardo Mayorga 2014-08-09 14:35:57 UTC
- You can drop Requires: python, it's not necessary.

- Documentation must be included in Python 3 subpackage as well.

Comment 8 Volker Fröhlich 2014-08-14 06:55:23 UTC
Thank you, Mayorga!

You didn't say which branches you want to create, but just in case you don't know: F20 has no python3-textile.

I was wondering whether the description is accurate and not misleading, in regards to "(these two are supported by default)."

However, APPROVED!

Comment 9 Christopher Meng 2014-08-14 07:41:33 UTC
I'm sorry, you all didn't realize the essential issues:

1. %global with_python3 1
%global srcname Markups

Name:           python-markups
Version:        0.5.0
Release:        3%{?dist}
Summary:        A wrapper around various text markups

License:        BSD
URL:            https://pypi.python.org/pypi/Markups
Source0:        https://pypi.python.org/packages/source/M/%{srcname}/%{srcname}-%{version}.tar.gz

BuildArch:      noarch
BuildRequires:  python2-devel
BuildRequires:  python-setuptools
BuildRequires:  python-markdown
BuildRequires:  python-docutils
BuildRequires:  python-textile
Requires:       python-markdown
Requires:       python-docutils
Requires:       python-textile
%if 0%{?with_python3}
BuildRequires:  python3-devel
BuildRequires:  python3-setuptools
BuildRequires:  python3-markdown
BuildRequires:  python3-docutils
BuildRequires:  python3-textile
Requires:       python3-markdown
Requires:       python3-docutils
Requires:       python3-textile
%endif

Yeah yeah, when you enable python3 build the python-markup will have python3 dependencies in itself whereas python3-markup has no python3 deps.

Please, move the %if 0%{?with_python3}...%endif to underneath the python3 %package.

2. rm -rf %{buildroot}/%{python3_sitelib}/*.egg-info

Please, I don't know what you are doing here, doing this will cause FTBFS of other packages. Volker is right, he told you to

(In reply to Volker Fröhlich from comment #2)
> Please delete the egg-info directory in the prep section. 

Which means the python eggs shipped in the tarball should be deleted _before_ %build, not to let you delete those generated by %build.

Comment 10 Christopher Meng 2014-08-14 07:44:14 UTC
s/to underneath/beneath/, sorry for the typo as I'm on iPad.

Comment 11 Nikos Roussos 2014-08-14 08:37:09 UTC
I have package several python packages before, but none so far with python3 subpackage. Can you elaborate on the dependency thing?

> Yeah yeah, when you enable python3 build the python-markup will have python3
> dependencies in itself whereas python3-markup has no python3 deps.
> 
> Please, move the %if 0%{?with_python3}...%endif to underneath the python3
> %package.

Why is that? I followed examples from other packages currently at Fedora that have the python3 dependencies along with python2 ones. For instance:
http://pkgs.fedoraproject.org/cgit/python-markdown.git/tree/python-markdown.spec
http://pkgs.fedoraproject.org/cgit/python-docutils.git/tree/python-docutils.spec

Thanks

Comment 12 Christopher Meng 2014-08-14 08:56:51 UTC
(In reply to Nikos Roussos from comment #11)
> http://pkgs.fedoraproject.org/cgit/python-markdown.git/tree/python-markdown.
> spec
> http://pkgs.fedoraproject.org/cgit/python-docutils.git/tree/python-docutils.
> spec

These 2 are correct, because they only put BuildRequires in the conditional blocks, not like this one from you, putting BuildRequires and Requires to wrong place exactly.

Your initial spec will give python-markups dependencies hereunder:

Requires:       python3-markdown
Requires:       python3-docutils
Requires:       python3-textile

And python2 packages shouldn't pull in any python3 deps, I think you should be able to understand this.

Comment 13 Nikos Roussos 2014-08-14 10:28:24 UTC
Thanks for the clarification. I think I get it now.

SPEC: https://comzeradd.fedorapeople.org/specs/python-markups.spec
SRPM: https://comzeradd.fedorapeople.org/srpms/python-markups-0.5.0-4.fc20.src.rpm

Comment 14 Christopher Meng 2014-08-14 12:54:28 UTC
Ok so the issue I found solved.

Hint: rm -rf %{srcname}-%{version}/Markups.egg-info is redundant, just rm -rf Markups.egg-info is enough I think(unless you really prefer old schools.)

As Volker hasn't posted any full review text, I won't set + here, Volker if you think this is OK, then let it go.

Comment 15 Christopher Meng 2014-08-19 06:41:14 UTC
I will mark my bug as duplicate of this one, although I'm the first.

Comment 16 Christopher Meng 2014-08-19 06:48:27 UTC
*** Bug 1119007 has been marked as a duplicate of this bug. ***

Comment 17 Nikos Roussos 2014-08-19 08:43:40 UTC
Thanks, I somehow missed this bug.

@Volker could you finalize the review process?

Comment 18 Volker Fröhlich 2015-04-21 15:43:45 UTC
Sorry, I'm constantly overworked.

Comment 19 Nikos Roussos 2016-01-16 23:16:16 UTC
This has already been packaged in the meantime on #1169493

*** This bug has been marked as a duplicate of bug 1169493 ***


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