https://bugzilla.redhat.com/show_bug.cgi?id=2141038
Bug ID: 2141038 Summary: Review Request: synce4l - SyncE implementation for Linux Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: mlichvar@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://fedorapeople.org/~mlichvar/tmp/synce4l.spec SRPM URL: https://fedorapeople.org/~mlichvar/tmp/synce4l-0-1.20221108git079577.fc38.sr... Description: synce4l is a software implementation of Synchronous Ethernet (SyncE) according to ITU-T Recommendation G.8264. The design goal is to provide logic to supported hardware by processing Ethernet Synchronization Messaging Channel (ESMC) and control Ethernet Equipment Clock (EEC) on Network Card Interface (NIC). Fedora Account System Username: mlichvar
https://bugzilla.redhat.com/show_bug.cgi?id=2141038
Benson Muite benson_muite@emailplus.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |benson_muite@emailplus.org Status|NEW |ASSIGNED Flags| |fedora-review? CC| |benson_muite@emailplus.org
https://bugzilla.redhat.com/show_bug.cgi?id=2141038
--- Comment #1 from Benson Muite benson_muite@emailplus.org --- 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]: 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", "GNU General Public License v2.0 or later", "*No copyright* GNU General Public License v2.0 or later", "*No copyright* GNU General Public License, Version 2". 5 files have unknown license. Detailed output of licensecheck in /home/FedoraPackaging/reviews/synce4l/2141038-synce4l/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: 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 [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [?]: Package use %makeinstall only when make install DESTDIR=... doesn't work. Note: %makeinstall used in %install section [x]: Package is named according to the Package Naming Guidelines. [?]: 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. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [x]: 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]: %config files are marked noreplace or the reason is justified. [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 is named using only allowed ASCII characters. [x]: No %config files under /usr. [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]: systemd_post is invoked in %post, systemd_preun in %preun, and systemd_postun in %postun for Systemd service files. Note: Systemd service file(s) in synce4l [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. [?]: Package should compile and build into binary rpms on all supported architectures. [-]: %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]: Spec use %global instead of %define unless justified.
===== EXTRA items =====
Generic: [!]: Spec file according to URL is the same as in SRPM. Note: Spec file as given by url is not the same as in SRPM (see attached diff). See: (this test has no URL) [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.
Rpmlint ------- Cannot parse rpmlint output:
Rpmlint (debuginfo) ------------------- Cannot parse rpmlint output:
Rpmlint (installed packages) ---------------------------- /bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8) /bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8) /bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8) ============================ rpmlint session starts ============================ rpmlint: 2.4.0 configuration: /usr/lib/python3.11/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 checks: 31, packages: 3
synce4l.x86_64: W: unused-direct-shlib-dependency /usr/sbin/synce4l /lib64/libm.so.6 synce4l.x86_64: W: incoherent-version-in-changelog 0-0.1.20221108git079577 ['0-1.20221108git079577.fc38', '0-1.20221108git079577'] 3 packages and 0 specfiles checked; 0 errors, 2 warnings, 0 badness; has taken 3.4 s
Source checksums ---------------- https://github.com/intel/synce4l/archive/079577/synce4l-079577.tar.gz : CHECKSUM(SHA256) this package : c8eb95caad8b57b4bac8beedb9a6b21c85866df5b0a1def4235eb7386cc9279f CHECKSUM(SHA256) upstream package : c8eb95caad8b57b4bac8beedb9a6b21c85866df5b0a1def4235eb7386cc9279f
Requires -------- synce4l (rpmlib, GLIBC filtered): /bin/sh config(synce4l) libc.so.6()(64bit) libm.so.6()(64bit) rtld(GNU_HASH) systemd
synce4l-debuginfo (rpmlib, GLIBC filtered):
synce4l-debugsource (rpmlib, GLIBC filtered):
Provides -------- synce4l: config(synce4l) synce4l synce4l(x86-64)
synce4l-debuginfo: debuginfo(build-id) synce4l-debuginfo synce4l-debuginfo(x86-64)
synce4l-debugsource: synce4l-debugsource synce4l-debugsource(x86-64)
Diff spec file in url and in SRPM --------------------------------- --- /home/benson/Projects/FedoraPackaging/reviews/synce4l/2141038-synce4l/srpm/synce4l.spec 2022-11-08 20:32:04.488872525 +0300 +++ /home/benson/Projects/FedoraPackaging/reviews/synce4l/2141038-synce4l/srpm-unpacked/synce4l.spec 2022-11-08 13:15:16.000000000 +0300 @@ -62,4 +62,4 @@
%changelog -* Tue Nov 08 2022 Miroslav Lichvar mlichvar@redhat.com 0-1.20221108git079577 +* Tue Nov 08 2022 Miroslav Lichvar mlichvar@redhat.com 0-0.1.20221108git079577 - make initial release
Generated by fedora-review 0.9.0 (6761b6c) last change: 2022-08-23 Command line :/usr/bin/fedora-review -b 2141038 Buildroot used: fedora-rawhide-x86_64 Active plugins: C/C++, Generic, Shell-api Disabled plugins: Haskell, Java, SugarActivity, Ruby, Python, fonts, Ocaml, R, Perl, PHP Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
Comments: a) Install commands are in the makefile, maybe a patch could be used to allow %make_install to work? Not essential though. b) Warning in build log: synce_port_ctrl.c:732:15: warning: implicit declaration of function 'pthread_setname_np'; did you mean 'pthread_setca nceltype'? [-Wimplicit-function-declaration] 732 | err = pthread_setname_np(*thread_id, thread_name); | ^~~~~~~~~~~~~~~~~~ | pthread_setcanceltype
Also
config.c:632:52: warning: '%s' directive output may be truncated writing up to 1839 bytes into a region of size 33 [- Wformat-truncation=] 632 | snprintf(buf, sizeof(buf), "global.%s", ci->label); | ^~ In file included from /usr/include/stdio.h:906, from config.c:25: In function 'snprintf', inlined from 'config_create' at config.c:632:3: /usr/include/bits/stdio2.h:54:10: note: '__snprintf_chk' output between 8 and 1847 bytes into a destination of size 4 0 54 | return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 55 | __glibc_objsize (__s), __fmt, | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 56 | __va_arg_pack ()); | ~~~~~~~~~~~~~~~~~ config.c: In function 'config_create': config.c:189:40: warning: '%s' directive output may be truncated writing up to 1839 bytes into a region of size 133 [ -Wformat-truncation=] 189 | snprintf(buf, sizeof(buf), "%s.%s", section, name); | ^~ In function 'snprintf', inlined from 'config_section_item' at config.c:189:2, inlined from 'config_global_item' at config.c:196:9, inlined from 'config_create' at config.c:642:8: /usr/include/bits/stdio2.h:54:10: note: '__snprintf_chk' output between 8 and 1847 bytes into a destination of size 1 40 54 | return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 55 | __glibc_objsize (__s), __fmt, | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 56 | __va_arg_pack ()); | ~~~~~~~~~~~~~~~~~
It seems you have patches for at least the _GNU_SOURCE, can this be added to what will be included here? Rather than modify the Makefile, one can also add a line at the top of synce_port_ctrl.c with
#ifdef _GNUC_ #define _GNU_SOURCE #endif
see for example: https://man7.org/linux/man-pages/man3/pthread_setname_np.3.html c) There are no tests in the code, perhaps have a smoke test such as ./synce4l -h
https://bugzilla.redhat.com/show_bug.cgi?id=2141038
--- Comment #2 from Miroslav Lichvar mlichvar@redhat.com --- Thanks for the review. I fixed the changelog, added a simple test and fixed one of the compiler warnings in:
Spec URL: https://fedorapeople.org/~mlichvar/tmp/synce4l.spec SRPM URL: https://fedorapeople.org/~mlichvar/tmp/synce4l-0-2.20221108git079577.fc38.sr...
As for the other issues, I think %makeinstall relies on the project having a configure script or something that would remember bindir, sbindir, mandir and all other paths as %makeinstall just sets DESTDIR.
The other compiler warning is in code taken from linuxptp, so I think I should try to fix it there first.
https://bugzilla.redhat.com/show_bug.cgi?id=2141038
--- Comment #3 from Benson Muite benson_muite@emailplus.org --- Ok. Fix removes first compiler warning. Can review again when second compiler warning is fixed - assume this should not be too troublesome and a patch can be sent upstream. It seems the codes for linuxptp and synce4l will be maintained independently.
https://bugzilla.redhat.com/show_bug.cgi?id=2141038
--- Comment #4 from Miroslav Lichvar mlichvar@redhat.com --- Spec URL: https://fedorapeople.org/~mlichvar/tmp/synce4l.spec SRPM URL: https://fedorapeople.org/~mlichvar/tmp/synce4l-0-3.20221114gitca51d5.fc38.sr...
This is an update to the latest git snapshot, which fixes the remaining compiler warnings.
https://bugzilla.redhat.com/show_bug.cgi?id=2141038
Benson Muite benson_muite@emailplus.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+ Status|ASSIGNED |POST
--- Comment #5 from Benson Muite benson_muite@emailplus.org --- A few rpmlint warnings: Rpmlint (installed packages) ---------------------------- /bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8) /bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8) /bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8) ============================ rpmlint session starts ============================ rpmlint: 2.4.0 configuration: /usr/lib/python3.11/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 checks: 31, packages: 3
synce4l.x86_64: W: unused-direct-shlib-dependency /usr/sbin/synce4l /lib64/libm.so.6 3 packages and 0 specfiles checked; 0 errors, 1 warnings, 0 badness; has taken 5.6 s
Package builds: https://copr.fedorainfracloud.org/coprs/fed500/synce4l/build/5042634/ Approved
https://bugzilla.redhat.com/show_bug.cgi?id=2141038
--- Comment #6 from Miroslav Lichvar mlichvar@redhat.com --- Thank you.
I've submitted an upstream PR for the unused-direct-shlib-dependency issue.
https://bugzilla.redhat.com/show_bug.cgi?id=2141038
--- Comment #7 from Gwyn Ciesla gwync@protonmail.com --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/synce4l
https://bugzilla.redhat.com/show_bug.cgi?id=2141038
Miroslav Lichvar mlichvar@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |RAWHIDE Last Closed| |2022-11-21 10:39:37
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com