Bug 1862625 - Review Request: ocaml-ppx-js-style - Code style checker for Jane Street OCaml packages
Summary: Review Request: ocaml-ppx-js-style - Code style checker for Jane Street OCaml...
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: 1862624
Blocks: 1862627
TreeView+ depends on / blocked
 
Reported: 2020-07-31 20:05 UTC by Jerry James
Modified: 2020-08-17 15:27 UTC (History)
4 users (show)

Fixed In Version: ocaml-ppx-js-style-0.14.0-2.fc33
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-08-17 15:27:13 UTC
Type: ---
Embargoed:
decathorpe: fedora-review+


Attachments (Terms of Use)

Description Jerry James 2020-07-31 20:05:15 UTC
Spec URL: https://jjames.fedorapeople.org/ocaml-ppx-js-style/ocaml-ppx-js-style.spec
SRPM URL: https://jjames.fedorapeople.org/ocaml-ppx-js-style/ocaml-ppx-js-style-0.14.0-1.fc33.src.rpm
Fedora Account System Username: jjames
Description: Ppx_js_style is an identity ppx rewriter that enforces Jane Street coding styles.

Comment 1 Fabio Valentini 2020-08-14 22:09:19 UTC
Taking this one too.

Comment 2 Fabio Valentini 2020-08-14 22:48:30 UTC
- latest version is packaged
- MIT license is correct
- package builds and installs successfully
- package conforms to OCaml Packaging Guidelines

Please adapt the BuildRequires: (foo and bar) to (foo with bar), as mentioned in the other Review Request.

The only thing I'm curious about is the 17.4 MB (?) ppx.exe file that's included in the main package.
What is it? .exe files are not mentioned in the OCaml Packaging Guidelines.
It also looks unreasonably large compared to the size of the source files.

(Curiously, despite building this package with "--enablerepo local" for it to pick up the fresh dependencies, the 555 permission issue isn't present.)

Comment 3 Jerry James 2020-08-16 01:51:41 UTC
(In reply to Fabio Valentini from comment #2)
> The only thing I'm curious about is the 17.4 MB (?) ppx.exe file that's
> included in the main package.
> What is it? .exe files are not mentioned in the OCaml Packaging Guidelines.
> It also looks unreasonably large compared to the size of the source files.

Thank you for asking that question.  It motivated me to finally figure out how to tell dune to show command lines as it builds projects.  Now I have another change to make to a bunch of OCaml packages.  (For the record, "dune --display=verbose" does the trick.)

This is the step that creates ppx.exe:

(cd _build/default && /usr/bin/ocamlopt.opt -g -o .ppx/0224ad3443a846e54f1637fccb074e7d/ppx.exe -w -24 -I /usr/lib64/ocaml/base -I /usr/lib64/ocaml/base/base_internalhash_types -I /usr/lib64/ocaml/base/caml -I /usr/lib64/ocaml/base/shadow_stdlib -I /usr/lib64/ocaml/compiler-libs -I /usr/lib64/ocaml/ocaml-compiler-libs/common -I /usr/lib64/ocaml/ocaml-compiler-libs/shadow -I /usr/lib64/ocaml/ocaml-migrate-parsetree -I /usr/lib64/ocaml/ppx_derivers -I /usr/lib64/ocaml/ppxlib -I /usr/lib64/ocaml/ppxlib/ast -I /usr/lib64/ocaml/ppxlib/metaquot -I /usr/lib64/ocaml/ppxlib/metaquot_lifters -I /usr/lib64/ocaml/ppxlib/print_diff -I /usr/lib64/ocaml/ppxlib/traverse_builtins -I /usr/lib64/ocaml/result -I /usr/lib64/ocaml/sexplib0 -I /usr/lib64/ocaml/stdio /usr/lib64/ocaml/compiler-libs/ocamlcommon.cmxa /usr/lib64/ocaml/ocaml-compiler-libs/common/ocaml_common.cmxa /usr/lib64/ocaml/ocaml-compiler-libs/shadow/ocaml_shadow.cmxa /usr/lib64/ocaml/result/result.cmxa /usr/lib64/ocaml/ppx_derivers/ppx_derivers.cmxa /usr/lib64/ocaml/ocaml-migrate-parsetree/migrate_parsetree.cmxa /usr/lib64/ocaml/ppxlib/ast/ppxlib_ast.cmxa /usr/lib64/ocaml/base/base_internalhash_types/base_internalhash_types.cmxa /usr/lib64/ocaml/base/caml/caml.cmxa /usr/lib64/ocaml/sexplib0/sexplib0.cmxa /usr/lib64/ocaml/base/shadow_stdlib/shadow_stdlib.cmxa /usr/lib64/ocaml/base/base.cmxa /usr/lib64/ocaml/stdio/stdio.cmxa /usr/lib64/ocaml/ppxlib/print_diff/ppxlib_print_diff.cmxa /usr/lib64/ocaml/ppxlib/traverse_builtins/ppxlib_traverse_builtins.cmxa /usr/lib64/ocaml/ppxlib/ppxlib.cmxa /usr/lib64/ocaml/ppxlib/metaquot_lifters/ppxlib_metaquot_lifters.cmxa /usr/lib64/ocaml/ppxlib/metaquot/ppxlib_metaquot.cmxa .ppx/0224ad3443a846e54f1637fccb074e7d/_ppx.ml)

So that executable is built out of a number of static libraries (cmxa) pulled from other OCaml packages, which is why it is so large.

New URLs with verbose builds and 'with' instead of 'and':
Spec URL: https://jjames.fedorapeople.org/ocaml-ppx-js-style/ocaml-ppx-js-style.spec
SRPM URL: https://jjames.fedorapeople.org/ocaml-ppx-js-style/ocaml-ppx-js-style-0.14.0-1.fc33.src.rpm

Comment 5 Fabio Valentini 2020-08-16 07:47:12 UTC
Great, good to know. Since those two points were the only things remaining from
https://bugzilla.redhat.com/show_bug.cgi?id=1862625#c2 :
Package is APPROVED

(If you want to swap reviews, I have a few open ones - and they should be easy, since they're "only" package renaming requests.)

Comment 6 Jerry James 2020-08-16 19:50:32 UTC
Thank you, Fabio.  Yes, I'm happy to do some reviews for you.

Comment 7 Fabio Valentini 2020-08-16 20:02:08 UTC
Great! I'll link the two that are currently blocking other work:

- stax-ex 1.7.7 → jaxb-stax-ex 1.8.3:
https://bugzilla.redhat.com/show_bug.cgi?id=1867981
(blocks xmlstreambuffer 1.5.9 update)

- glassfish-jax-rs-api → jakarta-ws-rs:
https://bugzilla.redhat.com/show_bug.cgi?id=1868994
(also fixes the package's FTBFS problem on f33+)

Thanks!

Comment 8 Gwyn Ciesla 2020-08-17 13:18:04 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/ocaml-ppx-js-style

Comment 9 Jerry James 2020-08-17 15:27:13 UTC
Built in F33 and Rawhide.


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