Bug 1266429 - Review Request: cmark - CommonMark parsing and rendering
Review Request: cmark - CommonMark parsing and rendering
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
: 1288649 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2015-09-25 05:17 EDT by Jens Petersen
Modified: 2016-05-21 01:19 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-02-05 16:21:32 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
jdulaney: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Jens Petersen 2015-09-25 05:17:37 EDT
Spec URL: http://petersen.fedorapeople.org//cmark.spec
SRPM URL: http://petersen.fedorapeople.org//cmark-0.22.0-1.fc22.src.rpm

Description:
`cmark` is the C reference implementation of CommonMark,
a rationalized version of Markdown syntax with a spec.

It provides a shared library (`libcmark`) with functions for parsing
CommonMark documents to an abstract syntax tree (AST), manipulating
the AST, and rendering the document to HTML, groff man, LaTeX,
CommonMark, or an XML representation of the AST.  It also provides a
command-line program (`cmark`) for parsing and rendering CommonMark
documents.
Comment 1 Upstream Release Monitoring 2015-09-25 05:20:12 EDT
petersen's scratch build of cmark-0.22.0-1.fc22.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11223872
Comment 2 Jens Petersen 2015-09-25 05:26:18 EDT
This is needed for the Haskell cmark library (to avoid bundling),
which in turn is needed by the latest version of pandoc
(for its CommonMark reader and writer I guess).
Comment 3 Miroslav Suchý 2015-10-01 07:42:38 EDT
- ldconfig called in %post and %postun if required.
  Note: /sbin/ldconfig not called in cmark-lib
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Shared_Libraries

- license of package is BSD however
    cmark-0.22.0/bench/statistics.py
  claims to be 
    *No copyright* Apache (v2.0)
  Hmm but you do not package it, and license is of binary package. So this is good.
Comment 4 Marco Driusso 2015-10-07 06:25:35 EDT
This is an unofficial review, for my sponsorship process.

Comments on the spec file:
- you are including a patch for fixing cmake lib64 problem, but actually not using it (no %patch macro in %prep section); moreover, I don't think that patch will fix the issue;
- instead of the mv workaround for fixing cmake lib64 issue, you may patch src/CMakeLists.txt by including GNUInstallDirs, that defines CMAKE_INSTALL_LIBDIR according to the system architecture (lib if i686, lib64 if x86_64); then, you can use CMAKE_INSTALL_LIBDIR in install(.. DESTINATION ..) directives;
- you should give a full valid URL in the Source0 tag;
- I would put both the command line binary and the versioned shared lib in the main package, and leave source headers and unversioned lib in the devel sub-package, without any lib sub-package;
- devel package must require the base package (the one containing the versioned shared lib); moreover, the dependency must be a fully versioned dependency (since there is also an architecture dependency);
- you should include additional documentation in the main package using %doc (e.g. the README.md file) in the %files section;
- as Miroslav pointed out, ldconfig must be called in %post and %postun sections, which should be placed after %check and before %files

You should then fix all errors and warnings highlighted by rpmlint (some related to the comments above):

$ rpmlint cmark.spec
cmark.spec:59: E: hardcoded-library-path in %{_prefix}/lib
cmark.spec: W: patch-not-applied Patch1: cmark-0.22.0-lib64.patch
cmark.spec: W: invalid-url Source0: cmark-0.22.0.tar.gz
0 packages and 1 specfiles checked; 1 errors, 2 warnings.

$ rpmlint cmark*.rpm
cmark.src: W: spelling-error %description -l en_US groff -> gruff, gr off, gr-off
cmark.src:59: E: hardcoded-library-path in %{_prefix}/lib
cmark.src: W: patch-not-applied Patch1: cmark-0.22.0-lib64.patch
cmark.src: W: invalid-url Source0: cmark-0.22.0.tar.gz
cmark.x86_64: W: spelling-error %description -l en_US groff -> gruff, gr off, gr-off
cmark-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/cmark-0.22.0/src/utf8.h
cmark-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/cmark-0.22.0/src/utf8.c
cmark-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/cmark-0.22.0/src/references.c
cmark-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/cmark-0.22.0/src/render.c
cmark-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/cmark-0.22.0/src/houdini_html_u.c
cmark-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/cmark-0.22.0/src/inlines.c
cmark-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/cmark-0.22.0/src/blocks.c
cmark-devel.x86_64: W: no-dependency-on cmark/cmark-libs/libcmark
cmark-devel.x86_64: W: spelling-error Summary(en_US) libcmark -> millibar
cmark-devel.x86_64: W: summary-not-capitalized C libcmark development files
cmark-devel.x86_64: W: spelling-error %description -l en_US libcmark -> millibar
cmark-devel.x86_64: W: only-non-binary-in-usr-lib
cmark-lib.x86_64: W: spelling-error %description -l en_US libcmark -> millibar
cmark-lib.x86_64: W: shared-lib-calls-exit /usr/lib64/libcmark.so.0.22.0 exit@GLIBC_2.2.5
cmark-lib.x86_64: W: no-documentation
cmark-lib.x86_64: E: library-without-ldconfig-postin /usr/lib64/libcmark.so.0.22.0
cmark-lib.x86_64: E: library-without-ldconfig-postun /usr/lib64/libcmark.so.0.22.0
5 packages and 0 specfiles checked; 3 errors, 19 warnings.
Comment 5 Michael Schwendt 2015-11-11 19:48:06 EST
True. Some good findings.


The lib64/lib fix is incomplete and doesn't cover the "libcmark.pc" pkg-config file yet. It specifies libdir=/usr/lib on x86_64, which is wrong.


> License:        BSD

The packaged file COPYING lists several more than BSD, such as MIT:

https://fedoraproject.org/wiki/Licensing:MIT?rd=Licensing/MIT#Modern_Style_with_sublicense

It seems it's the "Mixed Source Licensing Scenario":

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Mixed_Source_Licensing_Scenario
Comment 6 Upstream Release Monitoring 2015-12-04 13:46:47 EST
jdulaney's scratch build of cmark-0.22.0-1.fc23.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12055569
Comment 7 Jens Petersen 2016-01-20 04:25:39 EST
Thanks for the helpful review comments.
Apologies for neglecting this so long. :-(

In the meantime today I just noticed another review is also open (bug 1288649)...
anyway I hope we can merge/combine the effort.






(In reply to Miroslav Suchý from comment #3)
> - ldconfig called in %post and %postun if required.
>   Note: /sbin/ldconfig not called in cmark-lib

Thanks

(In reply to Marco Driusso from comment #4)
> - instead of the mv workaround for fixing cmake lib64 issue, you may patch
> src/CMakeLists.txt by including GNUInstallDirs

Thanks for this!  I struggled to find such a solution.
Including seems to do the right thing. :-)

> - you should give a full valid URL in the Source0 tag;

(I don't think it is possible for github releases unfortunately:
the url and the downloaded filname are different.)

> - I would put both the command line binary and the versioned shared lib in
> the main package, and leave source headers and unversioned lib in the devel
> sub-package, without any lib sub-package;

Perhaps you're right - it would make the packaging simpler.
I intentionall did it this way though since I wanted to give
the option to install without the program.
(eg cmake vs cmark: though it is only 5 chars)

> - devel package must require the base package (the one containing the
> versioned shared lib); moreover, the dependency must be a fully versioned
> dependency (since there is also an architecture dependency);

Yep

> - you should include additional documentation in the main package using %doc
> (e.g. the README.md file) in the %files section;

Yes, let me put it into the devel subpackage for now.


Thanks, these errors should all be fixed in:

Spec: http://petersen.fedorapeople.org/reviews/cmark/cmark.spec
SRPM: http://petersen.fedorapeople.org/reviews/cmark/cmark-0.23.0-1.fc23.src.rpm

(I updated to 0.23.0 since that is the version used in the Haskell library - though 0.24 may also work - upgrading this later should be trivial anyway.)
Comment 8 Jens Petersen 2016-01-20 04:27:38 EST
(I also updated the License field to include MIT also, thanks Michael.)
Comment 9 John Dulaney 2016-01-20 08:18:21 EST
You can do Source0 and URL as follows:

URL:           https://github.com/jgm/cmark
Source0:       https://github.com/jgm/cmark/archive/%{version}.tar.gz

This works.
Comment 10 Jens Petersen 2016-01-20 22:22:05 EST
(In reply to John Dulaney from comment #9)
> Source0:       https://github.com/jgm/cmark/archive/%{version}.tar.gz
> This works.

Not for me at least on F23:

$ rpmbuild -ba cmark.spec
error: File /home/pkgreview/libcmark/0.23.0.tar.gz: No such file or directory
Comment 11 Jens Petersen 2016-01-20 22:43:33 EST
Okay, thanks

Source0: https://github.com/jgm/cmark/archive/%{version}.tar.gz#/%{name}-%{version}.tar.gz

seems to work.

I feel https://fedoraproject.org/wiki/Packaging/SourceURL may be out of date since it is does not take github archive releases into account, but I didn't read all of https://fedorahosted.org/fpc/ticket/233 yet, but I think it
might have been written before the current github releases feature was
available or in widespread use at least.

Spec: http://petersen.fedorapeople.org/reviews/cmark/cmark.spec
SRPM: http://petersen.fedorapeople.org/reviews/cmark/cmark-0.23.0-2.fc23.src.rpm
Comment 12 Jens Petersen 2016-01-21 20:13:25 EST
(In reply to Jens Petersen from comment #11)
> I feel https://fedoraproject.org/wiki/Packaging/SourceURL may be out of date

(Well I think it is mostly about the case of when there is no release tarball.
I feel there should be specific coverage of github etc releases somewhere.)
Comment 13 John Dulaney 2016-01-22 13:24:31 EST
jdulaney@gefjon:~/tmp$ rpmlint ./cmark-0.23.0-2.fc23.src.rpm 
cmark.src: W: spelling-error %description -l en_US groff -> gruff, gr off, gr-off
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

rpmlint looks good, one last thing:  could you include the license in the lib so that it always gets installed?


As for source0 and github, have a look at my qtile spec:

http://pkgs.fedoraproject.org/cgit/rpms/qtile.git/tree/qtile.spec

The source0 url as specified works for qtile, as well as several other packages I maintain.
Comment 14 Jens Petersen 2016-01-25 03:55:03 EST
I aware of the form https://github.com/qtile/qtile/archive/v%{version}.tar.gz
but it downloads to qtile-0.10.4.tar.gz (at least from firefox): also I don't like unnamed tarballs.  Why github thinks this is cool is beyond me...

For my complicated url the downloaded filename and the last part of the url coincide/agree. :)
Comment 15 John Dulaney 2016-01-25 12:19:20 EST
Welp, go ahead and do the %license for lib and we're good.
Comment 16 Jens Petersen 2016-01-25 21:56:56 EST
Thanks - this should fix the missing license file from cmark-lib.

Spec: http://petersen.fedorapeople.org/reviews/cmark/cmark.spec
SRPM: http://petersen.fedorapeople.org/reviews/cmark/cmark-0.23.0-3.fc23.src.rpm
Comment 17 Upstream Release Monitoring 2016-01-26 08:52:16 EST
jdulaney's scratch build of cmark-0.23.0-3.fc23.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12689923
Comment 18 John Dulaney 2016-01-26 08:56:04 EST
Approved.
Comment 19 Jens Petersen 2016-01-27 04:12:34 EST
Thanks John!
Comment 20 John Dulaney 2016-01-27 04:34:03 EST
Quite so!
Comment 21 Gwyn Ciesla 2016-01-27 08:23:12 EST
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/cmark
Comment 22 Fedora Update System 2016-01-28 05:42:43 EST
cmark-0.23.0-3.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-cac4f03fc2
Comment 23 Fedora Update System 2016-01-28 05:42:44 EST
cmark-0.23.0-3.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-65d6af6d87
Comment 24 Fedora Update System 2016-01-28 05:42:51 EST
cmark-0.23.0-3.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2016-b9f659fc81
Comment 25 Fedora Update System 2016-01-28 20:53:02 EST
cmark-0.23.0-3.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-cac4f03fc2
Comment 26 Fedora Update System 2016-01-29 01:54:18 EST
cmark-0.23.0-3.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-b9f659fc81
Comment 27 Jens Petersen 2016-01-29 03:32:54 EST
John: I see that the haskell cmark binding was updated to use the latest release 0.24.1, so I am happy to update the version.  What do you need/want/use cmark for btw?
Comment 28 Fedora Update System 2016-01-29 04:07:29 EST
cmark-0.23.0-4.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-b3f7cd8978
Comment 29 Fedora Update System 2016-01-29 04:07:29 EST
cmark-0.23.0-4.el6 has been submitted as an update to Fedora EPEL 6. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-e102c14dc7
Comment 30 Fedora Update System 2016-01-29 11:20:46 EST
cmark-0.23.0-4.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-b3f7cd8978
Comment 31 Fedora Update System 2016-01-29 11:20:59 EST
cmark-0.23.0-3.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-65d6af6d87
Comment 32 Fedora Update System 2016-02-01 02:20:43 EST
cmark-0.23.0-4.el6 has been pushed to the Fedora EPEL 6 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-e102c14dc7
Comment 33 Fedora Update System 2016-02-05 16:21:29 EST
cmark-0.23.0-3.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.
Comment 34 Fedora Update System 2016-02-10 05:51:14 EST
cmark-0.23.0-3.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.
Comment 35 Fedora Update System 2016-03-05 20:26:37 EST
cmark-0.23.0-4.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.
Comment 36 Zbigniew Jędrzejewski-Szmek 2016-03-09 13:44:31 EST
*** Bug 1288649 has been marked as a duplicate of this bug. ***
Comment 37 Fedora Update System 2016-05-21 01:19:27 EDT
cmark-0.23.0-4.el6 has been pushed to the Fedora EPEL 6 stable repository. If problems still persist, please make note of it in this bug report.

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