Bug 1862624 - Review Request: ocaml-octavius - Ocamldoc comment syntax parser
Summary: Review Request: ocaml-octavius - Ocamldoc comment syntax parser
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Fabio Valentini
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1862625
TreeView+ depends on / blocked
 
Reported: 2020-07-31 20:05 UTC by Jerry James
Modified: 2020-08-14 21:30 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-08-14 21:30:54 UTC
Type: ---
Embargoed:
decathorpe: fedora-review+


Attachments (Terms of Use)

Description Jerry James 2020-07-31 20:05:11 UTC
Spec URL: https://jjames.fedorapeople.org/ocaml-octavius/ocaml-octavius.spec
SRPM URL: https://jjames.fedorapeople.org/ocaml-octavius/ocaml-octavius-1.2.2-1.fc33.src.rpm
Fedora Account System Username: jjames
Description: Octavius is an OCaml library to parse the ocamldoc comment syntax.

Comment 1 Fabio Valentini 2020-08-14 08:14:47 UTC
Taking this one.

Comment 2 Fabio Valentini 2020-08-14 08:37:30 UTC
Same as in one of the other OCaml packages, the Provides seem to be duplicated between the main and -devel package.

There also seem to be minor isses with the executable files, as shown by rpmlint:

ocaml-octavius.x86_64: W: ldd-failed /usr/bin/octavius
ocaml-octavius.x86_64: W: ldd-failed /usr/lib64/ocaml/octavius/octavius.cmxs
ocaml-octavius.x86_64: E: non-standard-executable-perm /usr/bin/octavius 555
ocaml-octavius.x86_64: E: non-standard-executable-perm /usr/lib64/ocaml/octavius/octavius.cmxs 555

Not sure if ldd is supposed to fail on OCaml binaries, but I don't think so :-)
And the executable permission should be 755, matching other files in /usr/bin.


Other than that, the package looks fine.

Comment 3 Jerry James 2020-08-14 17:17:30 UTC
Thanks for the review, Fabio.  That's weird.  Dune usually sets the correct permissions on files.  I don't know why it failed this time, but I have fixed them.  New URLs:

Spec URL: https://jjames.fedorapeople.org/ocaml-octavius/ocaml-octavius.spec
SRPM URL: https://jjames.fedorapeople.org/ocaml-octavius/ocaml-octavius-1.2.2-2.fc33.src.rpm

Comment 4 Fabio Valentini 2020-08-14 19:22:42 UTC
Well, I'm just hoping the 555 / 755 permission issues aren't too bad (either my PC is haunted or the glibc 2.33 dev snapshot is broken).

Package looks good now anyway.

- package is named correctly
- ISC license is correct
- latest version is packaged
- follows OCaml packaging guidelines

It might be good to compare builds in mock and in koji, to check whether binaries actually get chmodded 555 or 755 ...

Package is APPROVED.

Comment 5 Jerry James 2020-08-14 19:43:14 UTC
(In reply to Fabio Valentini from comment #4)
> It might be good to compare builds in mock and in koji, to check whether
> binaries actually get chmodded 555 or 755 ...

Yes, I'm going to do one build then wait to see the results in koji before I do any more.

> Package is APPROVED.

Thank you, Fabio!  I appreciate the review.

Comment 6 Gwyn Ciesla 2020-08-14 19:46:18 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/ocaml-octavius

Comment 7 Jerry James 2020-08-14 21:30:54 UTC
Built in F33 and Rawhide.


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