https://bugzilla.redhat.com/show_bug.cgi?id=2277254
--- Comment #3 from Matthew Krupcale mkrupcale@gmail.com --- Spec URL: https://download.copr.fedorainfracloud.org/results/mkrupcale/unison/fedora-r... SRPM URL: https://download.copr.fedorainfracloud.org/results/mkrupcale/unison/fedora-r...
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/743579... [3] https://github.com/bcpierce00/unison/issues/1032 [4] https://github.com/ocsigen/lwt/commit/ed71a00f0d5780234c870e9f62d36f62d899f5... [5] https://github.com/ocsigen/lwt/commit/b950e7b06d93b45593c141341db08e4df478ea... [6] https://github.com/vincenthz/ocaml-inotify/releases/tag/v1.0 [7] https://github.com/bcpierce00/unison/blob/060f54b0ec95e26df8725d699ef7b4cab2... [8] https://github.com/bcpierce00/unison/blob/060f54b0ec95e26df8725d699ef7b4cab2... [9] https://rpm-software-management.github.io/rpm/manual/conditionalbuilds.html