Bug 1862626 - Review Request: ocaml-ppx-optcomp - Optional compilation for OCaml
Summary: Review Request: ocaml-ppx-optcomp - Optional compilation for 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:
Blocks: 1862627
TreeView+ depends on / blocked
 
Reported: 2020-07-31 20:05 UTC by Jerry James
Modified: 2020-08-14 21:06 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2020-08-14 21:06:05 UTC
Type: ---
Embargoed:
decathorpe: fedora-review+


Attachments (Terms of Use)

Description Jerry James 2020-07-31 20:05:18 UTC
Spec URL: https://jjames.fedorapeople.org/ocaml-ppx-optcomp/ocaml-ppx-optcomp.spec
SRPM URL: https://jjames.fedorapeople.org/ocaml-ppx-optcomp/ocaml-ppx-optcomp-0.14.0-1.fc33.src.rpm
Fedora Account System Username: jjames
Description: Ppx_optcomp provides optional compilation for OCaml.  It is a tool used to handle optional compilations of pieces of code depending of the word size, the version of the compiler, etc.  The syntax is based on OCaml item extension nodes, with keywords similar to cpp.

Comment 1 Fabio Valentini 2020-08-14 08:39:41 UTC
Taking this one too.

Comment 2 Fabio Valentini 2020-08-14 10:19:01 UTC
Same issues here:

- duplicate Provides in main and -devel package (may not be problematic, as noted in other reviews)
- wrong file permission on executable (555 instead of 755)
- ldd fails to read /usr/lib64/ocaml/ppx_optcomp/ppx_optcomp.cmxs

Also your rich dependencies won't work the way you expect them to. "and" is the wrong operator, "with" is the correct one:

(ocaml-base-devel >= 0.14 and ocaml-base-devel < 0.15)   → (ocaml-base-devel >= 0.14 with ocaml-base-devel < 0.15)
(ocaml-stdio-devel >= 0.14 and ocaml-stdio-devel < 0.15) → (ocaml-stdio-devel >= 0.14 with ocaml-stdio-devel < 0.15)

Basically, the "with" ensures that the *same* dependency must satisfy both requirements.
*and* allows two different packages to satisfy the two requirements.

I assume you'll flip the "with test" bcond once the circular dependencies are all packaged?

Other than that, package looks good.

Comment 3 Jerry James 2020-08-14 17:41:15 UTC
(In reply to Fabio Valentini from comment #2)
> Same issues here:
> 
> - duplicate Provides in main and -devel package (may not be problematic, as
> noted in other reviews)
> - wrong file permission on executable (555 instead of 755)
> - ldd fails to read /usr/lib64/ocaml/ppx_optcomp/ppx_optcomp.cmxs

I don't understand why you are seeing wrong permissions.  I'm not.  In the dune source code, bin/install_uninstall.ml, the code that copies files into the install tree includes this:

  let copy_file ~src ~dst ~executable ~special_file ~package =
    let chmod =
      if executable then
        fun _ ->
      0o755
      else
        fun _ ->
      0o644

In other words, permissions are 755 for executables and 644 for everything else.  The spec file manually adds execute permissions to the cmxs (shared library) files.  And that's exactly what I see after doing a mock build of this package.  I do not see the wrong permissions you see.

I am running mock directly, rather than via fedora-review.  I wouldn't expect that to matter, but let me try the latter.  I'll report back.

> Also your rich dependencies won't work the way you expect them to. "and" is
> the wrong operator, "with" is the correct one:

Oh no!  Thanks for the heads up.  I'll have to fix that in a bunch of packages.  Thanks for alerting me before the rot spread any further.

> I assume you'll flip the "with test" bcond once the circular dependencies
> are all packaged?

That remains to be seen.  We have to figure out a strategy for building OCaml packages that participate in circular dependencies.  I've got some thoughts along those lines but need to flesh them out significantly.

Comment 4 Fabio Valentini 2020-08-14 17:47:47 UTC
Let me try again. I ran fedora-review with both "-m fedora-rawhide-x86_64" and "-o '--enablerepo local'" flags to build against the latest and greatest, but I've been noticing very weird buildroot issues with those settings for the past 2 days or so (see my strange Java build failures post to the devel list ...).
I'll check if dropping the "local" repo fixes the permissions ...

Comment 5 Fabio Valentini 2020-08-14 18:21:50 UTC
Uh, I think I found the issue. glibc 2.32.9000 might be broken. Running the mock build without "--enablerepo local" yields the correct permissions. :(

Comment 6 Fabio Valentini 2020-08-14 19:18:56 UTC
Well, I won't block the package review on unrelated issues.

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

Assuming the "and" is replaced with "with", package is APPROVED.

Comment 7 Jerry James 2020-08-14 19:36:38 UTC
(In reply to Fabio Valentini from comment #6)
> Assuming the "and" is replaced with "with", package is APPROVED.

Will do, for all of my OCaml packages.  Thank you, Fabio.

Comment 8 Gwyn Ciesla 2020-08-14 19:45:45 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/ocaml-ppx-optcomp

Comment 9 Jerry James 2020-08-14 21:06:05 UTC
Built in F33 and Rawhide.


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