https://bugzilla.redhat.com/show_bug.cgi?id=2277254 --- Comment #3 from Matthew Krupcale <mkrupcale@xxxxxxxxx> --- Spec URL: https://download.copr.fedorainfracloud.org/results/mkrupcale/unison/fedora-rawhide-x86_64/07435796-unison/unison.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/mkrupcale/unison/fedora-rawhide-x86_64/07435796-unison/unison-2.53.4-1.fc41.src.rpm Thank you for the thorough review Jerry! New builds are available with updates discussed below [1,2]. > This just means that you have to follow the unretirement process instead of > the new package process. Will do. > - This package contains a bundled copy of ocaml-lwt in src/lwt. Can it be > unbundled? It appears to be a very old copy of ocaml-lwt, possible even > older than 1.1.0, which is the oldest tagged version at > https://github.com/ocsigen/lwt. I discussed this with upstream [3], and the situation is actually the opposite: Lwt was created for Unison and then extracted as a separate project, and both histories have since then diverged significantly. Thus, it would not be feasible to use upstream Lwt. From what I can tell, the license at the time (2008) Lwt was extracted was LGPL-2.1-or-later [4,5], so I've indicated as such in the spec. Should I still indicate "Provides: bundled(ocaml-lwt)"? > - This package contains a bundled copy of ocaml-inotify in > src/fsmonitor/inotify (see https://github.com/vincenthz/ocaml-inotify). > We do not have that package in Fedora. You should either create an > ocaml-inotify package and unbundle it from unison, or add > "Provides: bundled(ocaml-inotify)" (plus "= version" if the version can be > determined) and make sure the License field represents the license of this > code. Added "Provides: bundled(ocaml-inotify)". The version is unclear and is actually modified by unison upstream, whereas upstream ocaml-inotify was last released (v1.0) on 2010-02-01 [6]. > - This package contains code borrowed from OCaml itself in src/ubase/myMap.* > and src/hash_compat.c. The License field must reflect that. Updated License. src/ubase/myMap.* appears to be LGPL-2.0-only, derived from an older OCaml stdlib, while src/hash_compat.c is stated as LGPL-2.1-only. > - This package contains code borrowed from Xavier Leroy in src/ubase/uarg.*. > The original license of this code will have to be determined and included in > the License field. Appears to similarly be LGPL-2.0-only, derived from an older OCaml stdlib. > - Change all instances of "%{__install}" to "install". See > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_macros Done. > - In src/fswatchold.ml, function watchercmd attempts to find and run > fsmonitor.py. This package does not install fsmonitor.py. Will that be a > problem? No, fsmonitor.py need not be installed when when unison-fsmonitor is built (and it is built) [7], and if fsmonitor.py is not installed, fswatchold.ml will not attempt to execute it with the python interpreter [8]. > - Please add a %check script. It just needs to run "make test". Done. > - See the name-repeated-in-summary warnings from rpmlint below. Perhaps the > word "Unison" could be omitted from each Summary line? Done. > - RPM now supports a less confusing %bcond syntax. Consider replacing the > first two lines of the file with: > %bcond doc 1 > %bcond gtk 1 Done. Looks to be a rpm v4.17.1 feature [9], but EPEL 9 (rpm v4.16.1) interestingly seems to still build with this syntax [2]. > - Consider using %autorelease and %autochangelog. See > https://docs.fedoraproject.org/en-US/packaging-guidelines/#changelogs I'm maybe still a little bit old-school here and kind of prefer the greater control over the release version and changelog. I feel like the %changelog should reflect more user-facing package changes, whereas I tend to git commit granular spec changes that may have no user-facing impact. > - Consider changing "BuildRequires: texlive-latex" to > "BuildRequires: tex-latex". The virtual provide will remain available even > if we switch from TeXLive to some other TeX distribution in the future. Done. > - Consider making unison.desktop be Source1. That way, the timestamp doesn't > change on every build. Done. > - Consider adding a metainfo file (which this package SHOULD have). See > https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/ Done, as Source2. > - Consider including the changelog of the previous iteration of the unison > package, to give credit to those who worked on it before. Done, using the %changelog from most recent unison240.spec before orphaning. [1] https://copr.fedorainfracloud.org/coprs/mkrupcale/unison/build/7435796/ [2] https://copr.fedorainfracloud.org/coprs/mkrupcale/rhel-9-unison/build/7435797/ [3] https://github.com/bcpierce00/unison/issues/1032 [4] https://github.com/ocsigen/lwt/commit/ed71a00f0d5780234c870e9f62d36f62d899f553 [5] https://github.com/ocsigen/lwt/commit/b950e7b06d93b45593c141341db08e4df478eae3 [6] https://github.com/vincenthz/ocaml-inotify/releases/tag/v1.0 [7] https://github.com/bcpierce00/unison/blob/060f54b0ec95e26df8725d699ef7b4cab23e0d68/Makefile#L68 [8] https://github.com/bcpierce00/unison/blob/060f54b0ec95e26df8725d699ef7b4cab23e0d68/src/fswatchold.ml#L30 [9] https://rpm-software-management.github.io/rpm/manual/conditionalbuilds.html -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2277254 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202277254%23c3 -- _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
