Bug 1230963 - Review Request: mkdocs - Python tool to create HTML documentation from markdown sources
Summary: Review Request: mkdocs - Python tool to create HTML documentation from markdo...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jonathan Underwood
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1230968
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-06-11 21:48 UTC by William Moreno
Modified: 2015-07-27 21:01 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-07-27 21:01:23 UTC
jonathan.underwood: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description William Moreno 2015-06-11 21:48:07 UTC
Spec URL: http://rmsconsultoresnicaragua.com/rpmdev/mkdocs.spec
SRPM URL: http://rmsconsultoresnicaragua.com/rpmdev/mkdocs-0.14.0-1.fc22.src.rpm
Description: Project documentation with Markdown
Fedora Account System Username: williamjmorenor

Comment 1 William Moreno 2015-07-04 22:50:36 UTC
Spec URL: https://williamjmorenor.fedorapeople.org/rpmdev/mkdocs.spec
SRPM URL: https://williamjmorenor.fedorapeople.org/rpmdev/mkdocs-0.14.0-1.fc22.src.rpm

=====
Just change to fedora-peopel host

Comment 2 William Moreno 2015-07-04 22:59:00 UTC
The source have a doc dir with the source to build documentation in html, this docs require mkdocs to translate the markdown to html, I will build the documentation and include it in a doc subpackage when mkdocs become available in comps.

It will work very similar to build html documentation in %%build with python-sphinx.

Comment 3 Jonathan Underwood 2015-07-08 20:30:31 UTC
Before I get stuck into a full review, some nits:

1) Package summary is uninformative to a user unfamiliar with the package. It sounds like a package containing documentation.

2) Description is badly line wrapped and justified

3) Description is also not very clear - it's not at all obvious that "static site" is referring to a website, and that this package produces html.

4) Since you're targetting only Fedora with this spec file, the %{!?_licensedir:%global license %%doc} is unnecessary.

Comment 4 William Moreno 2015-07-08 22:32:05 UTC
Spec URL: https://williamjmorenor.fedorapeople.org/rpmdev/mkdocs.spec
SRPM URL: https://williamjmorenor.fedorapeople.org/rpmdev/mkdocs-0.14.0-2.fc22.src.rpm

You know than Summary and Description were copied and pasted from upstream website, well a good summary and description will have user to find the tool, by the way I make rpmlint happy and now I only get one warning about the missing manpage than it is reported to upstream:

https://github.com/mkdocs/mkdocs/issues/686

I have a buildroot override for python-certifi but I need the buildroot override for livereload to build the package in koji.

Comment 5 Jonathan Underwood 2015-07-08 22:54:26 UTC
First para in description is still poorly wrapped:

MkDocs is a fast and simple way to create a website from source files written 
in Markdown, and configured with a single YAML configuration file, the 
documentation can be hosted anywhere, even in free 
hosting services as Read the Docs and GitHub Pages.

--->The last line of that para is broken too early.

Also, the last line of the description:

MkDocs is Python powered, this package is build with Python 3.

contains a typo. "build" should be "built".

Comment 7 Jonathan Underwood 2015-07-09 10:32:44 UTC
OK, there's a bundling issue here - the themes all include highlight.pack.js which seems to be a minified highlight.js. This is available as the package js-highlight. So, you need to remove the bundled versions and you probably need to symlink to the js-highlight file for each theme. And of course, Require js-highlight.

Well done on unbundling the fontawesome font already though!

Comment 8 Jonathan Underwood 2015-07-09 10:39:21 UTC
Hm, actually, you also haven't completely unbundled the fontawesome fonts.

The .woff file is available in fontawesome-fonts-web.

The .otf and .ttf are available in fontawesome-fonts

So, those files need removing from your package, and symlinks added if required.

Comment 9 Jonathan Underwood 2015-07-09 10:42:22 UTC
Or, rather than symlinks, you may be able to adjust the config or patch the python files to use the system locations for the .js and font files. %{python3_sitelib} really shouldn't have font and js files, or symlinks to them. Fonts and js files belong under /usr/share really.

Comment 10 Dougal Matthews 2015-07-09 11:22:03 UTC
FWIW, the themes have gone under large restructuring in the master branch pending the next release. I hope it doesn't make life more difficult for you.

Comment 11 Jonathan Underwood 2015-07-09 12:23:26 UTC
(In reply to Dougal Matthews from comment #10)
> FWIW, the themes have gone under large restructuring in the master branch
> pending the next release. I hope it doesn't make life more difficult for you.

Hopefully upstream has also moved data to /usr/share :).

Comment 12 Jonathan Underwood 2015-07-09 12:35:59 UTC
Actually I see support for --install-data in setup.py, so it may well be there already - something to investigate.

Comment 13 Dougal Matthews 2015-07-09 12:56:27 UTC
(In reply to Jonathan Underwood from comment #11)
> (In reply to Dougal Matthews from comment #10)
> > FWIW, the themes have gone under large restructuring in the master branch
> > pending the next release. I hope it doesn't make life more difficult for you.
> 
> Hopefully upstream has also moved data to /usr/share :).

I am upstream :) We have not done anything to explicitly support this...


(In reply to Jonathan Underwood from comment #12)
> Actually I see support for --install-data in setup.py, so it may well be
> there already - something to investigate.

... so it only exists if it is a standard setup.py feature.

Comment 14 Jonathan Underwood 2015-07-09 14:34:25 UTC
(In reply to Dougal Matthews from comment #13)
> (In reply to Jonathan Underwood from comment #11)
> > (In reply to Dougal Matthews from comment #10)
> > > FWIW, the themes have gone under large restructuring in the master branch
> > > pending the next release. I hope it doesn't make life more difficult for you.
> > 
> > Hopefully upstream has also moved data to /usr/share :).
> 
> I am upstream :) We have not done anything to explicitly support this...
> 
> 
> (In reply to Jonathan Underwood from comment #12)
> > Actually I see support for --install-data in setup.py, so it may well be
> > there already - something to investigate.
> 
> ... so it only exists if it is a standard setup.py feature.

Right. A quick test passing --install-data %{buildroot}%{_datadir}/%{name} to the install command didn't move the data to /usr/share, sadly. I think this really needs to happen for FHS compliance... all the directories such as assets, themes, templates etc really don't belong under site-lib/site-packges, but rather under /usr/share/mkdocs.

Comment 15 William Moreno 2015-07-09 14:39:09 UTC
If nice to see upstream following the review :) And in the other hand will be great to have upstream as comantainer of the package once it have been aproved.

I will have to work in removing the blunded js and fonts one per time and test is the Requires is good enough or I will need to simlink them, so I do not want to post a incomplete unbundling maybe I will update the review for weekend.

I will try to work in the manpage and send the PR in GitHub to help in upstream with that, also could be really great than at future MkDocs can make a manpage from the markdown sources.

Comment 16 Jonathan Underwood 2015-07-09 16:24:14 UTC
OK, that's fine William, I'll continue the review whenever you're ready. Noting what Dougal mentioned about the next release having a lot of restructuring, it may be more fruitful to package up a snapshot of the master development branch instead, if a new release is imminent.

Comment 17 Dougal Matthews 2015-07-09 16:33:12 UTC
(In reply to Jonathan Underwood from comment #16)
> OK, that's fine William, I'll continue the review whenever you're ready.
> Noting what Dougal mentioned about the next release having a lot of
> restructuring, it may be more fruitful to package up a snapshot of the
> master development branch instead, if a new release is imminent.

It isn't imminent, I would expect it to happen within a month, but it is hard to say.

Comment 18 William Moreno 2015-07-10 05:05:50 UTC
Spec URL: https://williamjmorenor.fedorapeople.org/rpmdev/mkdocs.spec
SRPM URL: https://williamjmorenor.fedorapeople.org/rpmdev/mkdocs-0.14.0-4.fc22.src.rpm

I have removed bundled fonst and hightligh.pack.js and simlink to system files and mkdocs is working!

I made a loop to simlink the fonst and js in every theme, but I wrote each theme name by hand in the loop so I wan to ask if you can recomend a way to get the themes names with out need to write every theme name by hand.

======
for theme in "amelia" "bootstrap" "cerulean" "cosmo" "cyborg" "flatly" \
"journal" "mkdocs" "readable" "readthedocs" "simplex" "slate" "spacelab" \
"united" "yeti"
do 
=====

I wrote a base manpage and it is proposed to upstream, with this there is nor rpmlint messages.

I have opened a bug in python-tornado to have all depencies in f22 and not only in rawhide

https://bugzilla.redhat.com/show_bug.cgi?id=1241771

Comment 19 Jonathan Underwood 2015-07-10 09:55:36 UTC
Something based on this might be what you're after:

for i in mkdocs/themes/* ; do d=`realpath $i` ; echo $d ; done


Or, you could use find, with something like 

find mkdocs/themes -maxdepth 1 -mindepth 1 -type d

Comment 20 Jonathan Underwood 2015-07-23 17:13:14 UTC
Hi William - how's this one shaping up?

Comment 21 William Moreno 2015-07-23 22:43:33 UTC
Spec URL: https://williamjmorenor.fedorapeople.org/rpmdev/mkdocs.spec
SRPM URL: https://williamjmorenor.fedorapeople.org/rpmdev/mkdocs-0.14.0-4.fc22.src.rpm

----
- Remove bundled fonts and js
- Add manpage

rpmlint show a lot of loud in the rpm about broken simlinks but once the package is installed this is fixed so the linked file exist in the spected patch, with this simliks mkdocs work fine, even I am using it rigth now to write a small doc.

Comment 22 Jonathan Underwood 2015-07-24 22:50:36 UTC
The spec file changelog has been badly line wrapped such that the version-release tag appears as the first entry - that needs fixing. You generally seem to have an issue with line wrapping (as seen in other reviews) - I suggest you turn line wrapping off in whatever editor you use as it is wasting you a lot of time.

I see some unused code commented out in the spec file, but often that causes issues if the first character on a line after the # is a %. In general it is good practice to remove unused code from the spec file to aid readability and allow other packagers to help maintain the package.

I see that moving the themes to /usr/share/... hasn't happened, such that the package isn't FHS compliant - this really does need fixing.

Comment 23 William Moreno 2015-07-24 23:45:41 UTC
(In reply to Jonathan Underwood from comment #22)
> The spec file changelog has been badly line wrapped such that the
> version-release tag appears as the first entry - that needs fixing. You
> generally seem to have an issue with line wrapping (as seen in other
> reviews) - I suggest you turn line wrapping off in whatever editor you use
> as it is wasting you a lot of time.

No, the changelog it is in prescribed format, see the 3rd options:
https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
> 
> I see some unused code commented out in the spec file, but often that causes
> issues if the first character on a line after the # is a %. In general it is
> good practice to remove unused code from the spec file to aid readability
> and allow other packagers to help maintain the package.

This is a easy fix, it is not a bloquer isue

> I see that moving the themes to /usr/share/... hasn't happened, such that
> the package isn't FHS compliant - this really does need fixing.

I need to refers to python-sphinx as the most similar package to mkdocs than I can find and python-sphinx place theme files under /usr/lib/python2.7/site-packages/sphinx/themes/

So I really thinks than move theme files to /usr/share it is not required

See:
http://koji.fedoraproject.org/koji/rpminfo?fileStart=350&rpmID=6560137&fileOrder=name&buildrootOrder=-id&buildrootStart=50#filelist

Debian packaging also do not move the templates files:
https://packages.debian.org/source/sid/python-mkdocs

Arch packging also do not do it:
https://aur.archlinux.org/packages/python-mkdocs/

The arch packaging it is not very good by the way :(

Comment 24 William Moreno 2015-07-24 23:55:21 UTC
See also python-sphinx_rtd_theme

The theme files are in %{python3_sitelib}/%{pkgname}* and %{python2_sitelib}/%{pkgname}*

http://pkgs.fedoraproject.org/cgit/python-sphinx_rtd_theme.git/tree/python-sphinx_rtd_theme.spec

Comment 25 Jonathan Underwood 2015-07-25 00:26:21 UTC
(In reply to William Moreno from comment #23)
> (In reply to Jonathan Underwood from comment #22)
> > The spec file changelog has been badly line wrapped such that the
> > version-release tag appears as the first entry - that needs fixing. You
> > generally seem to have an issue with line wrapping (as seen in other
> > reviews) - I suggest you turn line wrapping off in whatever editor you use
> > as it is wasting you a lot of time.
> 
> No, the changelog it is in prescribed format, see the 3rd options:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
> > 

Ah, yes, I hadn't realized that format was acceptable, I stand corrected. I have to say though, I find it harder to read.

> > I see some unused code commented out in the spec file, but often that causes
> > issues if the first character on a line after the # is a %. In general it is
> > good practice to remove unused code from the spec file to aid readability
> > and allow other packagers to help maintain the package.
> 
> This is a easy fix, it is not a bloquer isue
> 

Yep.

> > I see that moving the themes to /usr/share/... hasn't happened, such that
> > the package isn't FHS compliant - this really does need fixing.
> 
> I need to refers to python-sphinx as the most similar package to mkdocs than
> I can find and python-sphinx place theme files under
> /usr/lib/python2.7/site-packages/sphinx/themes/
> 
> So I really thinks than move theme files to /usr/share it is not required
> 
> See:
> http://koji.fedoraproject.org/koji/
> rpminfo?fileStart=350&rpmID=6560137&fileOrder=name&buildrootOrder=-
> id&buildrootStart=50#filelist
> 
> Debian packaging also do not move the templates files:
> https://packages.debian.org/source/sid/python-mkdocs
> 
> Arch packging also do not do it:
> https://aur.archlinux.org/packages/python-mkdocs/
> 
> The arch packaging it is not very good by the way :(


That all may be true, but nonetheless, FHS compliance is a blocker (and sphinx should also be fixed). Data should go under /usr/share.

Comment 26 Jonathan Underwood 2015-07-25 00:38:13 UTC
I have started a thread on the Fedora packaging list to clarify this - hopefully that'll get things resolved :).

Comment 27 Jonathan Underwood 2015-07-25 15:00:49 UTC
Actually, it seems Debian sphinx packaging has moved to putting all the none python stuff such as themes under /usr/share:

https://packages.debian.org/sid/all/sphinx-common/filelist

Comment 28 Jonathan Underwood 2015-07-26 00:45:11 UTC
OK, I raised this upstream here:

https://github.com/mkdocs/mkdocs/issues/697

it seems this whole issue is an unfortunate side-effect of mkdocs reliance on entry points and that setuptools breaks the distutils expectations of where data files are installed. The net result seems to be that without re-engineering the whole python packaging stack we're stuck with this situation. So, although it pains me greatly to see such filesystem abuse, I'll APPROVE the package, but please do keep an eye on the situation and see if ti can be resolved more satisfactorily in the future.

Comment 29 William Moreno 2015-07-26 03:08:32 UTC
Python developers do the best they can to be sure their apps will run but pip do not track dependencies out pypi so Python apps bundled many thinks like .js, fonts, and others. More developers will ensure their app work out of the box from pypi than in a specific distro.

Thanks for the review.

Comment 30 William Moreno 2015-07-26 03:14:21 UTC
New Package SCM Request
=======================
Package Name: mkdocs
Short Description: Python tool to create HTML documentation from markdown sources
Upstream URL: http://www.mkdocs.org/
Owners: williamjmorenor
Branches: master
InitialCC: williamjmorenor

Comment 31 William Moreno 2015-07-27 04:02:12 UTC
New Package SCM Request
=======================
Package Name: mkdocs
Short Description: Python tool to create HTML documentation from markdown sources
Upstream URL: http://www.mkdocs.org/
Owners: williamjmorenor
Branches: master f23
InitialCC: williamjmorenor

Comment 32 Gwyn Ciesla 2015-07-27 19:16:16 UTC
Git done (by process-git-requests).


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