Spec URL: https://download.copr.fedorainfracloud.org/results/solomoncyj/safestring/fedora-rawhide-x86_64/08108224-safestringlib/softstring.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/solomoncyj/safestring/fedora-rawhide-x86_64/08108224-safestringlib/safestringlib-1.2.020240520%5Efd159bf-1.fc42.src.rpm Description: Routines for safe string operations Fedora Account System Username: solomoncyj Reproducible: Always
https://copr.fedorainfracloud.org/coprs/solomoncyj/safestring/build/8108224/
The name of the specfile is wrong, change it to match the srpm The install fedora-review and do fedora-review -n safestringlib And resolve the issues. URL: https://github.com/intel/safestringlib Source0: https://github.com/gianni-rosato/svt-av1-psy/archive/refs/heads/master.zip Source0 is not safestringlib, it is a video codec.
This is a review *template*. Besides handling the [ ]-marked tests you are also supposed to fix the template before pasting into bugzilla: - Add issues you find to the list of issues on top. If there isn't such a list, create one. - Add your own remarks to the template checks. - Add new lines marked [!] or [?] when you discover new things not listed by fedora-review. - Change or remove any text in the template which is plain wrong. In this case you could also file a bug against fedora-review - Remove the "[ ] Manual check required", you will not have any such lines in what you paste. - Remove attachments which you deem not really useful (the rpmlint ones are mandatory, though) - Remove this text Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - Header files in -devel subpackage, if present. Note: safestringlib : /usr/include/safe_lib.h safestringlib : /usr/include/safe_lib_errno.h safestringlib : /usr/include/safe_mem_lib.h safestringlib : /usr/include/safe_str_lib.h safestringlib : /usr/include/safe_types.h safestringlib : /usr/include/snprintf_s.h See: https://docs.fedoraproject.org/en-US/packaging- guidelines/#_devel_packages - Development (unversioned) .so files in -devel subpackage, if present. Note: Unversioned so-files directly in %_libdir. See: https://docs.fedoraproject.org/en-US/packaging- guidelines/#_devel_packages ===== MUST items ===== C/C++: [ ]: Package does not contain kernel modules. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: ldconfig not called in %post and %postun for Fedora 28 and later. [x]: Package does not contain any libtool archives (.la) [x]: Package contains no static executables. [x]: Rpath absent or only used for internal libs. Generic: [ ]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [ ]: 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. [ ]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "MIT License", "*No copyright* MIT License". 14 files have unknown license. Detailed output of licensecheck in /home/solomoncyj/Downloads/safestring/review- safestringlib/licensecheck.txt [ ]: Package does not own files or directories owned by other packages. Note: Dirs in package are owned also by: /usr/include(filesystem) [ ]: %build honors applicable compiler flags or justifies otherwise. [ ]: Package contains no bundled libraries without FPC exception. [ ]: Changelog in prescribed format. [ ]: Sources contain only permissible code or content. [ ]: Package contains desktop file if it is a GUI application. [ ]: Development files must be in a -devel package [ ]: Package uses nothing in %doc for runtime. [ ]: Package consistently uses macros (instead of hard-coded directory names). [ ]: Package is named according to the Package Naming Guidelines. [ ]: Package does not generate any conflict. [ ]: 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. [ ]: Spec file is legible and written in American English. [ ]: Package contains systemd file(s) if in need. [ ]: Useful -debuginfo package or justification otherwise. [ ]: Package is not known to require an ExcludeArch tag. [ ]: 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]: The License field must be a valid SPDX expression. [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [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]: 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]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 0 bytes in 0 files. [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. [ ]: Final provides and requires are sane (see attachments). [ ]: Package functions as described. [ ]: Latest version is packaged. [ ]: 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. [ ]: Package should compile and build into binary rpms on all supported architectures. [ ]: %check is present and all tests pass. [ ]: 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]: 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]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [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 ------- Checking: safestringlib-1.2.020240520^fd159bf-1.fc42.x86_64.rpm safestringlib-1.2.020240520^fd159bf-1.fc42.src.rpm ============================ rpmlint session starts ============================ rpmlint: 2.5.0 configuration: /usr/lib/python3.13/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml rpmlintrc: [PosixPath('/tmp/tmp2ec66qak')] checks: 32, packages: 2 safestringlib.src: W: summary-not-capitalized routines for safe string operations safestringlib.x86_64: W: summary-not-capitalized routines for safe string operations safestringlib.x86_64: E: standard-dir-owned-by-package /usr/include safestringlib.src: E: spelling-error ('strcpy', '%description -l en_US strcpy -> stripy') safestringlib.src: E: spelling-error ('memcpy', '%description -l en_US memcpy -> memory') safestringlib.x86_64: E: spelling-error ('strcpy', '%description -l en_US strcpy -> stripy') safestringlib.x86_64: E: spelling-error ('memcpy', '%description -l en_US memcpy -> memory') safestringlib.x86_64: W: no-documentation safestringlib.x86_64: W: devel-file-in-non-devel-package /usr/include/safe_lib.h safestringlib.x86_64: W: devel-file-in-non-devel-package /usr/include/safe_lib_errno.h safestringlib.x86_64: W: devel-file-in-non-devel-package /usr/include/safe_mem_lib.h safestringlib.x86_64: W: devel-file-in-non-devel-package /usr/include/safe_str_lib.h safestringlib.x86_64: W: devel-file-in-non-devel-package /usr/include/safe_types.h safestringlib.x86_64: W: devel-file-in-non-devel-package /usr/include/snprintf_s.h safestringlib.x86_64: W: devel-file-in-non-devel-package /usr/lib/libsafestring_shared.so 2 packages and 0 specfiles checked; 5 errors, 10 warnings, 23 filtered, 5 badness; has taken 0.4 s Rpmlint (debuginfo) ------------------- Checking: safestringlib-debuginfo-1.2.020240520^fd159bf-1.fc42.x86_64.rpm ============================ rpmlint session starts ============================ rpmlint: 2.5.0 configuration: /usr/lib/python3.13/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml rpmlintrc: [PosixPath('/tmp/tmpp2gj669j')] checks: 32, packages: 1 1 packages and 0 specfiles checked; 0 errors, 0 warnings, 5 filtered, 0 badness; has taken 0.2 s Rpmlint (installed packages) ---------------------------- ============================ rpmlint session starts ============================ rpmlint: 2.5.0 configuration: /usr/lib/python3.13/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 2 safestringlib.x86_64: W: summary-not-capitalized routines for safe string operations safestringlib.x86_64: E: standard-dir-owned-by-package /usr/include safestringlib.x86_64: E: spelling-error ('strcpy', '%description -l en_US strcpy -> stripy') safestringlib.x86_64: E: spelling-error ('memcpy', '%description -l en_US memcpy -> memory') safestringlib.x86_64: W: no-documentation safestringlib.x86_64: W: devel-file-in-non-devel-package /usr/include/safe_lib.h safestringlib.x86_64: W: devel-file-in-non-devel-package /usr/include/safe_lib_errno.h safestringlib.x86_64: W: devel-file-in-non-devel-package /usr/include/safe_mem_lib.h safestringlib.x86_64: W: devel-file-in-non-devel-package /usr/include/safe_str_lib.h safestringlib.x86_64: W: devel-file-in-non-devel-package /usr/include/safe_types.h safestringlib.x86_64: W: devel-file-in-non-devel-package /usr/include/snprintf_s.h safestringlib.x86_64: W: devel-file-in-non-devel-package /usr/lib/libsafestring_shared.so 2 packages and 0 specfiles checked; 3 errors, 9 warnings, 24 filtered, 3 badness; has taken 0.5 s Unversioned so-files -------------------- safestringlib: /usr/lib/libsafestring_shared.so Source checksums ---------------- https://github.com/intel/safestringlib/archive/refs/heads/master.zip : CHECKSUM(SHA256) this package : c2b2417f76c081e0716a80582b06592e253f8317f1b88cb61fe6f15aa3e57349 CHECKSUM(SHA256) upstream package : c2b2417f76c081e0716a80582b06592e253f8317f1b88cb61fe6f15aa3e57349 Requires -------- safestringlib (rpmlib, GLIBC filtered): libc.so.6()(64bit) libsafestring_shared.so.1.2.0()(64bit) rtld(GNU_HASH) Provides -------- safestringlib: libsafestring_shared.so.1.2.0()(64bit) safestringlib safestringlib(x86-64) Generated by fedora-review 0.10.0 (e79b66b) last change: 2023-07-24 Command line :/usr/bin/fedora-review -n safestringlib Buildroot used: fedora-rawhide-x86_64 Active plugins: Shell-api, Generic, C/C++ Disabled plugins: Perl, SugarActivity, fonts, Haskell, R, PHP, Ocaml, Java, Python Disabled flags: EXARCH, EPEL6, EPEL7, DISTTAG, BATCH
Spec URL: https://download.copr.fedorainfracloud.org/results/solomoncyj/safestring/fedora-rawhide-x86_64/08110193-safestringlib/safestringlib.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/solomoncyj/safestring/fedora-rawhide-x86_64/08110193-safestringlib/safestringlib-1.2.020240520%5Efd159bf-1.fc42.src.rpm Description: Routines for safe string operations Fedora Account System Username: solomoncyj
Better. Now fix the issues that fedora-review reports. do not worry about the spelling errors for strcpy etc. or lack of documentation
Copr build: https://copr.fedorainfracloud.org/coprs/build/8110345 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2316473-safestringlib/fedora-rawhide-x86_64/08110345-safestringlib/fedora-review/review.txt Found issues: - safestringlib : /usr/include/safe_lib.h safestringlib : /usr/include/safe_lib_errno.h safestringlib : /usr/include/safe_mem_lib.h safestringlib : /usr/include/safe_str_lib.h safestringlib : /usr/include/safe_types.h safestringlib : /usr/include/snprintf_s.h Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages - Unversioned so-files directly in %_libdir. Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
please wait, patching upstream cmake
Spec URL: https://download.copr.fedorainfracloud.org/results/solomoncyj/safestring/fedora-eln-x86_64/08111773-safestringlib/safestringlib.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/solomoncyj/safestring/fedora-eln-x86_64/08111773-safestringlib/safestringlib-1.2.020240520%5Efd159bf-1.eln143.src.rpm Description: Routines for safe string operations Fedora Account System Username: solomoncyj
Created attachment 2050721 [details] The .spec file difference from Copr build 8110345 to 8111778
Copr build: https://copr.fedorainfracloud.org/coprs/build/8111778 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2316473-safestringlib/fedora-rawhide-x86_64/08111778-safestringlib/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
@ Tom.Rix
A few things to point out: - Do not override `%_libdir`. For arch dependent projects, libraries must be installed under `/usr/lib64` which is the default - The project is by definition a C-only project. Please patch in `LANGUAGES C` under `project` [1] and remove the `gcc-c++` dependency - `ctest` were not executed. Please add `-DBUILD_UNITTESTS=ON` to make sure they are compiled and run (navigate to the `Executing(%check)` section to see if they are actually executed) - Do not wildcard Licenses. It is important to keep track of license changes and that would obfuscate things - Please use macros for the snapshot details (commit date and hash). You can use `%forgemeta` macros [2] or just manually define `%commit` and `%snapshot_date` [3] - I prefer not to include `%{?epoch:%{epoch}:}` until it is actually needed, but some may have better reasoning if/when it's needed early on - Preference: use a more constrained glob pattern for the `%_includedir`. Something like `*.h` is fine imo, but others prefer to list all files explicitly - Preference: some (me included) prefer to separate sections by 2 blank lines and order them in the execution order: `%prep` -> `%generate_buildrequires` -> (`%conf` only available since 4.18 ->) `%build` -> `%install` -> `%check` (also `%files` and `%changelong` even though they are not similar scriplets) Furthermore could you contact upstream about packaging issues, and you can ping me on those as a person of contact for CMake stuff: - Use `CMakePackageConfigHelpers` macros [4] and include the `safestringlibVersion.cmake` file - Do not manually add compilation flags (it interferes with our optimization flags), instead move them to presets and only run them in the CI - Bump the minimum cmake version (if the minimum is too low, recent CMake changes and imporvements are not applied) - Consider migrating the includes into dedicated subfolder [1]: https://github.com/intel/safestringlib/blob/fd159bf8c37a2a0b7ba328766f3c96d9aa075f27/CMakeLists.txt#L7-L10 [2]: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_using_forges_hosted_revision_control [3]: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_hosting_services [4]: https://cmake.org/cmake/help/latest/module/CMakePackageConfigHelpers.html
Blocked: - removing g++ causes project cmake to refuse to configure - waiting for upstreqa to cut a pr / commit/relese to fix the cmake file (I am bad at cmake)
https://copr.fedorainfracloud.org/coprs/solomoncyj/safestring/build/8131430/
> - removing g++ causes project cmake to refuse to configure I was very confused to see the builds failing, and I debugged it locally. I have missed that there is another usage of `project()`[1], you need to add `LANGUAGES C` in there as well for the same reason. > - waiting for upstreqa to cut a pr / commit/relese to fix the cmake file (I am bad at cmake) It is not required to wait for upstream for such patches. About the new spec file, there is mostly one issue with: > mv /usr/lib/libsafestring_shared.so* /usr/lib64/ This command would not work because the intermediate install directory is `%{buildroot}/usr/lib64/...`, but also the install path is already set correctly to `/usr/lib64`, so you don't need to worry about it. You can very this using `mock /path/to/srpm` and then enter the mock environment with `mock --shell` and you will find the stuff under `/build/BUILD...` [1]: https://github.com/intel/safestringlib/blob/master/unittests/CMakeLists.txt
https://copr.fedorainfracloud.org/coprs/solomoncyj/safestring/build/8135360/
Spec URL: https://download.copr.fedorainfracloud.org/results/solomoncyj/safestring/fedora-40-x86_64/08135360-safestringlib/safestringlib.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/solomoncyj/safestring/fedora-40-x86_64/08135360-safestringlib/safestringlib-1.2.020240520%5Efd159bf-1.fc40.src.rpm Description: Routines for safe string operations Fedora Account System Username: solomoncyj
Final 3 blocking change requests: - Don't add a new line between `%description` and the body. Try to run `rpm -qi ...` you will see that it is being included there. Generally it is common design to remove the other separations between section headers (%build, %install, etc.) and their body below - Seems upstream does not include `ctest` tests. In the meantime run it manually via `%{__cmake_builddir}/unittests/safestring_test` (you may need to prefix it with `./`?) - Add a `^` separator between the version `1.2.0` and the snapshotdate [1]. Also using a macro for it would be preferred Other than that it looks fine. Various issues with the project itself, e.g. the naming of the library, lack of `SOVERSION` etc. but I will pick those bones with upstream directly personally since backwards compatibility is tricky. [1]: https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_snapshots
Spec URL: https://download.copr.fedorainfracloud.org/results/solomoncyj/safestring/fedora-40-x86_64/08137148-safestringlib/safestringlib.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/solomoncyj/safestring/fedora-40-x86_64/08137148-safestringlib/safestringlib-1.2.0%5E20240520fd159bf-1.fc40.src.rpm Description: Routines for safe string operations Fedora Account System Username: solomoncyj
Welp the tests are not exiting on error when they should be, e.g.: ``` test_strisalphanumeric_s 50 Error rc=1 ``` is reporting an error with: https://github.com/intel/safestringlib/blob/fd159bf8c37a2a0b7ba328766f3c96d9aa075f27/unittests/test_strisalphanumeric_s.c#L45-L51 Maybe if upstream can back to us on those issues, try to ping them directly about the test failures, it seems `dmwheel1` and `tomasbw` might be responsible for the project. At the same time, I've looked through the issues and one of the issues [1] is recommending https://github.com/rurban/safeclib/ as an alternative as well. It could also be an alternative and it shouldn't matter much which version is actually used since the API are compatible (just the CMake code to consume it will be different). How about we try to contact the devs for both projects and ask their recommendation on which version is more suitable for packaging. Either `safestringlib` devs would come back and comment on the failing tests, or the `safeclib` devs could offer some support for packaging that instead. [1]: https://github.com/intel/safestringlib/issues/2