Spec URL: https://jonathanspw.fedorapeople.org/ImHex.spec
SRPM URL: https://jonathanspw.fedorapeople.org/ImHex-1.20.0-2.fc37.src.rpm

Updated to exclude s390x (unsupported upstream) and fix my name in the changelog.

Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=90503181

> BuildRequires:  mesa-libGL-devel

You'll likely want `libglvnd-devel` instead, as it is the actual provider of OpenGL headers/libraries.

> # [11:38 AM] WerWolv: Officially supported are x86_64 and aarch64
> ExcludeArch:    i686
> ExcludeArch:    armv7hl

%{ix86} and %{arm32} correspondingly. But given the upstream support status, it would make sense to do `ExclusiveArch: x86_64 %{arm64}`. And ppc64le if you are sure it works there.

> make -C redhat-linux-build -j unit_tests

Both `make` and `redhat-linux-build` are %cmake macro implementation details that may change. Maybe `%cmake_build -- --target unit_tests` would work?


 - Please, also declare `Provides: bundled()` for all third-party libraries built in the package: yara (conditionally), capstone, imgui, libromfs, microtar, nativefiledialog, pattern_language, xdgpp... Now that I listed all of that, I feel that the License tag is incomplete :)
Also, consider unbundling at least some of those.


 - As it is a GUI app, it should provide an AppStream metainfo file (otherwise you won't be able to find and install ImHex with gnome-software). See https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/

Thanks for taking a look.

(In reply to Aleksei Bavshin from comment #3)
> > BuildRequires:  mesa-libGL-devel
> 
> You'll likely want `libglvnd-devel` instead, as it is the actual provider of
> OpenGL headers/libraries.

Fixed.

> 
> > # [11:38 AM] WerWolv: Officially supported are x86_64 and aarch64
> > ExcludeArch:    i686
> > ExcludeArch:    armv7hl
> 
> %{ix86} and %{arm32} correspondingly. But given the upstream support status,
> it would make sense to do `ExclusiveArch: x86_64 %{arm64}`. And ppc64le if
> you are sure it works there.

Swapped to using ExclusiveArch and macros where possible.

> > make -C redhat-linux-build -j unit_tests
> 
> Both `make` and `redhat-linux-build` are %cmake macro implementation details
> that may change. Maybe `%cmake_build -- --target unit_tests` would work?

Indeed this works!  Thank you!

>  - Please, also declare `Provides: bundled()` for all third-party libraries
> built in the package: yara (conditionally), capstone, imgui, libromfs,
> microtar, nativefiledialog, pattern_language, xdgpp... Now that I listed all
> of that, I feel that the License tag is incomplete :)
> Also, consider unbundling at least some of those.

Do they need to be listed as bundled even if none of them actually end up installed in the filesystem anywhere and are only used to build?

This application is very bleeding edge in terms of the libs it uses and some of the things simply can't be packaged separately yet.  capstone for example hasn't release v5 but the packaged version is basically v5 which is required for ImHex.

A few of the other things like pattern_language and libromfs I fully intend to package separately.  They're from the same upstream developer so I was planning to do that a bit later on as to not delay getting this package itself into the repos.

Upstream has to do some work to permit system libs for some of these things to be used such as proper versioning and releases.  All possible system libs are being used with the cmake flags currently.  yara is about to be non-conditional too as the latest yara is being pushed back to f36 I'm just waiting on bodhi to push it to stable.

>  - As it is a GUI app, it should provide an AppStream metainfo file
> (otherwise you won't be able to find and install ImHex with gnome-software).
> See https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/

Added.

Thanks for all your suggestions.

Spec URL: https://jonathanspw.fedorapeople.org/ImHex.spec
SRPM URL: https://jonathanspw.fedorapeople.org/ImHex-1.20.0-3.fc37.src.rpm

> Do they need to be listed as bundled even if none of them actually end up installed in the filesystem anywhere and are only used to build?

Yes. If something is compiled and included into the packaged binary, it should affect both the binary license and the list of bundled packages. The reason for the former should be obvious, and the latter is mostly a means of detecting code duplication from the repository metadata.

Corresponding guidelines with a deeper explanations:
https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling (which also asks to identify the version of a bundled library, if possible)
https://docs.fedoraproject.org/en-US/legal/license-field/

And since I haven't checked the docs above in a while, I forgot to ask to rm -rf already unbundles external libs in %prep. This way we can guarantee that the bundled sources are not used.

> Added.

Looks good to me. I'm assuming you are already aware that the Software app can find and view the metainfo from the installed package, even when it's still not available in the repositories (and most importantly in the `appstream-data` package, which is what G-S actually consumes). If not, might be good to check.

Nit: I always assumed that the "project_license" should include the project license without the (bundled) dependencies ("The license given in the project_license tag should be the ‘main’ license of the project"[1]).
I might be interpreting this wrong, so feel free to ask legal@ or just leave it as is.


[1]: https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html#tag-project_license

Implemented your suggestions.

Spec URL: https://jonathanspw.fedorapeople.org/ImHex.spec
SRPM URL: https://jonathanspw.fedorapeople.org/ImHex-1.20.0-3.fc37.src.rpm

Checked it in GS and it looks great :)

Looks like you've already gotten quite a bit of feedback! Don't hate me but I see one thing in the spec review...

New packages have to be in all lower case:

https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_common_character_set_for_package_naming

Now rereading it says SHOULD not MUST/SHALL. Hmm...

I was following upstream on that.  I prefer things in lower-case, but I figured following upstream made the most sense here.

Either way results in the same usage of macros.  Had all lower-case let me use more macros I'd have gone with that.

I'll probably flip to all lower for final packaging/release since the docs recommend it.

@hobbes1069 does everything look good to you or would you like to see additional changes?

I'll let you worry about the case of the package name :) Working on the rest of the review. I think 90% will be the licenses involved.

So I would say to simplify our other conversation, the license of the package is based on what distributed in the package (not build system files and other irrelevant stuff). That being said some of the text files such as READMEs have some specific licenses which is not something I've considered before.

So overall I'm okay with the licensing but there does seem to be some MIT stuff. Are you sure they're not being included?

I've got the review mostly completed but want to make sure we're both good on the licensing. This may be a pain but I think any of the bundled libraries should have their license file included:

$ grep LICENSE licensecheck.txt 
ImHex/lib/libimhex-rs/imgui-rs/LICENSE-APACHE
ImHex/lib/libimhex-rs/imgui-sys/LICENSE-APACHE
ImHex/lib/external/nativefiledialog/LICENSE
ImHex/lib/external/capstone/LICENSE.TXT
ImHex/lib/external/capstone/bindings/python/LICENSE.TXT
ImHex/lib/external/capstone/suite/regress/LICENSE
ImHex/LICENSE
ImHex/lib/external/pattern_language/LICENSE
ImHex/resources/LICENSE.rtf
ImHex/resources/romfs/LICENSE
ImHex/lib/external/microtar/LICENSE
ImHex/lib/external/pattern_language/external/fmt/LICENSE.rst
ImHex/lib/external/pattern_language/external/intervaltree/LICENSE
ImHex/lib/external/xdgpp/LICENSE
ImHex/lib/libimhex-rs/imgui-rs/LICENSE-MIT
ImHex/lib/libimhex-rs/imgui-sys/LICENSE-MIT
ImHex/lib/external/capstone/LICENSE_LLVM.TXT

In some cases they may be identical, so either include one? Or maybe just a little bash scripting in %install:

cp ImHex/lib/libimhex-rs/imgui-rs/LICENSE-APACHE ./imgui-rs-LICENSE-APACHE
etc...

And then include in %license.

Thoughts?

Here's what I've come up with:

Spec URL: https://jonathanspw.fedorapeople.org/imhex.spec
SRPM URL: https://jonathanspw.fedorapeople.org/imhex-1.20.0-3.fc38.src.rpm

That looks good enough for me. I can hopefully finish the review after work today.

*** APPROVED ***

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig not called in %post and %postun for Fedora 28 and later.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated", "GNU General Public License, Version
     2", "*No copyright* GNU General Public License, Version 2", "BSD
     3-Clause License", "University of Illinois/NCSA Open Source License",
     "MIT License", "*No copyright* MIT License", "*No copyright* zlib
     License", "*No copyright* Apache License 2.0", "*No copyright*
     [generated file]", "*No copyright* The Unlicense [generated file]",
     "The Unlicense MIT License", "Apache License", "*No copyright* MIT
     License CNRI Python License", "CNRI Python Open Source GPL Compatible
     License Agreement". 1880 files have unknown license. Detailed output
     of licensecheck in /home/build/fedora-
     review/2115901-ImHex/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[ ]: %build honors applicable compiler flags or justifies otherwise.
[-]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[-]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 20480 bytes in 1 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install or
     desktop-file-validate if there is such a file.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: There are rpmlint messages (see attachment).
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Cannot parse rpmlint output:


Rpmlint (debuginfo)
-------------------
Cannot parse rpmlint output:



Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:


Source checksums
----------------
https://github.com/WerWolv/ImHex/releases/download/v1.20.0/Full.Sources.tar.gz#/ImHex-1.20.0.tar.gz :
  CHECKSUM(SHA256) this package     : 014469fe11ff52fa02c91db810e23cd94be37cc48f6cfd9207873567011ea0b3
  CHECKSUM(SHA256) upstream package : 014469fe11ff52fa02c91db810e23cd94be37cc48f6cfd9207873567011ea0b3


Requires
--------
ImHex (rpmlib, GLIBC filtered):
    ld-linux-x86-64.so.2()(64bit)
    libOpenGL.so.0()(64bit)
    libc.so.6()(64bit)
    libcurl.so.4()(64bit)
    libdbus-1.so.3()(64bit)
    libdbus-1.so.3(LIBDBUS_1_3)(64bit)
    libfmt.so.9()(64bit)
    libfreetype.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgcc_s.so.1(GCC_3.4)(64bit)
    libgcc_s.so.1(GCC_4.2.0)(64bit)
    libglfw.so.3()(64bit)
    libimhex.so.1.20.0()(64bit)
    libm.so.6()(64bit)
    libmagic.so.1()(64bit)
    libmbedcrypto.so.7()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.11)(64bit)
    libstdc++.so.6(CXXABI_1.3.13)(64bit)
    libstdc++.so.6(CXXABI_1.3.2)(64bit)
    libstdc++.so.6(CXXABI_1.3.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.5)(64bit)
    libstdc++.so.6(CXXABI_1.3.9)(64bit)
    libyara.so.9()(64bit)
    rtld(GNU_HASH)

ImHex-debuginfo (rpmlib, GLIBC filtered):

ImHex-debugsource (rpmlib, GLIBC filtered):



Provides
--------
ImHex:
    ImHex
    ImHex(x86-64)
    application()
    application(imhex.desktop)
    bundled(capstone)
    bundled(gnulib)
    bundled(imgui)
    bundled(libpl)
    bundled(libromfs)
    bundled(microtar)
    bundled(nativefiledialog)
    libimhex.so.1.20.0()(64bit)
    metainfo()
    metainfo(imhex.metainfo.xml)

ImHex-debuginfo:
    ImHex-debuginfo
    ImHex-debuginfo(x86-64)
    debuginfo(build-id)
    libimhex.so.1.20.0-1.20.0-3.fc37.x86_64.debug()(64bit)

ImHex-debugsource:
    ImHex-debugsource
    ImHex-debugsource(x86-64)



Generated by fedora-review 0.8.0 (e988316) last change: 2022-04-07
Command line :/usr/bin/fedora-review -b 2115901
Buildroot used: fedora-rawhide-x86_64
Active plugins: Shell-api, Generic, C/C++
Disabled plugins: fonts, Haskell, Ocaml, SugarActivity, PHP, Java, R, Python, Perl
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/imhex

https://bodhi.fedoraproject.org/updates/FEDORA-2022-1c203dbe49