Spec URL: http://dl.kwizart.net/review/wdt.spec SRPM URL: http://dl.kwizart.net/review/wdt-1.32.1910230-1.20200909gitb585d21.fc31.src.rpm Description: WDT is aiming to transfer data between 2 systems as fast as possible Fedora Account System Username: kwizart koji scratch build: f32 https://koji.fedoraproject.org/koji/taskinfo?taskID=54052800 el8 https://koji.fedoraproject.org/koji/taskinfo?taskID=54051822
As of a couple of days ago, we have folly packaged in Fedora, so you should be able to link against that instead of embedding it here. See #1887621 and https://src.fedoraproject.org/rpms/folly for details.
Taking this review
Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed ===== MUST items ===== C/C++: [X]: Package does not contain kernel modules. [!]: 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", "Apache License 2.0", "*No copyright* Apache License 2.0", "BSD 3-clause "New" or "Revised" License", "Expat License", "*No copyright* [generated file]", "GNU General Public License, Version 2", "zlib/libpng license". 365 files have unknown license. Detailed output of licensecheck in /home/davide/1891040-wdt/licensecheck.txt [X]: License file installed when any subpackage combination is installed. [X]: %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. [-]: 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. [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. [!]: 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]: 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). [X]: 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. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [!]: 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]: 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: [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: wdt-1.32.1910230-1.20200909gitb585d21.fc34.x86_64.rpm wdt-debuginfo-1.32.1910230-1.20200909gitb585d21.fc34.x86_64.rpm wdt-debugsource-1.32.1910230-1.20200909gitb585d21.fc34.x86_64.rpm wdt-1.32.1910230-1.20200909gitb585d21.fc34.src.rpm wdt.x86_64: W: name-repeated-in-summary C WDT wdt.x86_64: W: no-manual-page-for-binary wcp wdt.x86_64: W: no-manual-page-for-binary wdt wdt.src: W: name-repeated-in-summary C WDT wdt.src:62: E: hardcoded-library-path in %{_prefix}/lib 4 packages and 0 specfiles checked; 1 errors, 4 warnings. Rpmlint (debuginfo) ------------------- Checking: wdt-debuginfo-1.32.1910230-1.20200909gitb585d21.fc34.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- wdt.x86_64: W: name-repeated-in-summary C WDT wdt.x86_64: W: invalid-url URL: https://www.facebook.com/WdtOpenSource <urlopen error [Errno -3] Temporary failure in name resolution> wdt.x86_64: W: no-manual-page-for-binary wcp wdt.x86_64: W: no-manual-page-for-binary wdt wdt-debuginfo.x86_64: W: invalid-url URL: https://www.facebook.com/WdtOpenSource <urlopen error [Errno -3] Temporary failure in name resolution> wdt-debugsource.x86_64: W: invalid-url URL: https://www.facebook.com/WdtOpenSource <urlopen error [Errno -3] Temporary failure in name resolution> 3 packages and 0 specfiles checked; 0 errors, 6 warnings. Source checksums ---------------- https://github.com/facebook/folly/archive/v2020.10.19.00/folly-2020.10.19.00.tar.gz : CHECKSUM(SHA256) this package : 47954f5587226f14b946db51e76846ab4fbc7b419aba742a9ba67d27be8e90bf CHECKSUM(SHA256) upstream package : 47954f5587226f14b946db51e76846ab4fbc7b419aba742a9ba67d27be8e90bf https://github.com/facebook/wdt/archive/b585d21ccb500899c697cc3f37b16f1ccd1a2a31/wdt-b585d21.tar.gz : CHECKSUM(SHA256) this package : aab9eebb35c670a9e080dc9e796aa3e56678e906af572e70076cea769ca41492 CHECKSUM(SHA256) upstream package : aab9eebb35c670a9e080dc9e796aa3e56678e906af572e70076cea769ca41492 Requires -------- wdt (rpmlib, GLIBC filtered): /usr/bin/bash libc.so.6()(64bit) libcrypto.so.1.1()(64bit) libcrypto.so.1.1(OPENSSL_1_1_0)(64bit) libdouble-conversion.so.3()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_3.3.1)(64bit) libgflags.so.2.2()(64bit) libglog.so.0()(64bit) libm.so.6()(64bit) libpthread.so.0()(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.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) rtld(GNU_HASH) wdt-debuginfo (rpmlib, GLIBC filtered): wdt-debugsource (rpmlib, GLIBC filtered): Provides -------- wdt: bundled(folly) wdt wdt(x86-64) wdt-debuginfo: debuginfo(build-id) wdt-debuginfo wdt-debuginfo(x86-64) wdt-debugsource: wdt-debugsource wdt-debugsource(x86-64) Generated by fedora-review 0.7.5 (5fa5b7e) last change: 2020-02-16 Command line :/usr/bin/fedora-review -b 1891040 Buildroot used: fedora-rawhide-x86_64 Active plugins: C/C++, Shell-api, Generic Disabled plugins: PHP, Python, Ocaml, fonts, Perl, Haskell, SugarActivity, Java, R Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
- as mentioned above, please add a BR to folly-devel instead of bundling it - wcp is build is a static executable, and the built is currently setup to disable shared libraries; I don't think there's a reason to do a static build here - folly doesn't build on s390x, so wdt will need an ExcludeArch for it - wdt has tests that should be run in %check (see BUILD_TESTING in CMakeLists.txt) - I think you need a BR on sed, given that you're using it in %prep - rpmlint looks ok, the only thing I'd fix there is the summary, instead of "WDT is aiming to transfer" I'd say "tool aiming to transfer" to avoid the repetition
subscribing in case we need to change anything in folly's packaging. There used to be an explicit list of packages that don't have to be added as BR, but it doesn't seem to be in the current guidelines anymore. Adding a BR for sed is probably a good idea, though it does not seem to be needed right now.
Hello Davide, Michel, Thanks for looking into this. Wdt seems to assume folly to be built in a "sister directory" from the source tree, so it looks like there is "some amount of work" to switch to use a system folly. I'm not sure to be able to work on such feature by the end of the month due to daily work. Building as shared lib looks possible, but this affect both folly and wdt, so if building with shared, there is probably a need to unbundle folly first. For tests, there is a need to build them in the first step, but they are failing. I've managed, to build them locally, but not on koji as apparently using BUILD_TESTING=ON expect to have internet access in order to download from gtest git. Once downloaded I've made few compilation fixes See also https://github.com/facebook/wdt/pull/210 I've also fixed an issue with gtest locally, but eventually this could be fixed by https://github.com/facebook/wdt/pull/210 (not tested). All in all, seems it will be difficult to contine with this submittion, it might be easier to withdraw this review request so you can continue with a new submission.
I think the main thing to sort out here is the bundling of folly -- once that's wrangled, the shared library build should follow fairly easily. I wouldn't worry too much about fixing the tests for now, but I do recommend wiring them up in specfile behind a bcond; that'll make it easier to fix them later on. For gtest, in theory it should be enough to add it as a BR instead of downloading it.
Spec URL: http://dl.kwizart.net/review/wdt.spec SRPM URL: http://dl.kwizart.net/review/wdt-1.32.1910230-2.20200909gitb585d21.fc31.src.rpm Description: Tool aiming to transfer data between 2 systems as fast as possible Changelog: - Improve description - Add --with tests conditional for tests - Switch to cmake3 - And ExcludeArch inherited from folly koji scratch build for f34: https://koji.fedoraproject.org/koji/taskinfo?taskID=55112390 For some reason I'm not able to run the test (and experience the failure anymore). I only see "No tests were found!!!" There is still a need to unbundle folly, but I welcome help with this.
Actually, I think the Summary would be better with "Warp speed Data Transfer" which is what wdt stands for.
Thanks! Agreed on the Summary. On my end, I'm trying to get some of the PRs on the upstream repo merged to make this easier. Notably, I think https://github.com/facebook/wdt/pull/197 should help with the folly unbundling.
Ok, all pending PRs are merged upstream now. If you build with WDT_USE_SYSTEM_FOLLY=ON you should be able to link against the folly BR instead of the built in one.
Going to take this over as it turns out wdt required some pretty major work upstream to get in shape enough to be usable here. Hope you don't mind Nicolas, happy to add you as co-maintainer if you'd like.
Spec URL: https://dcavalca.fedorapeople.org/review/wdt/wdt.spec SRPM URL: https://dcavalca.fedorapeople.org/review/wdt/wdt-1.32.1910230-1.20210128git6aec23c.fc34.src.rpm Changelog: - Bump git commit to include upstream fixes - Use system folly instead of bundling it - Default to shared library build - Use %%cmake instead of %%cmake3 - Use %%bcond for tests and turn off by default due to flakiness - Update BuildRequires and Requires
Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=60767348
Fine with me, please take over this package... On the "process side", you really need to create another ticket for the review. That's because having the submitter for the review and the ticket "reporter" for the bug need to be the same person. This is enforced as the verification check one the infra side, after the review is approved. Thanks for heading this, and sorry for the lack of answer. (busy as hell theses days).
Thanks! I've filed a new ticket at https://bugzilla.redhat.com/show_bug.cgi?id=1922315 *** This bug has been marked as a duplicate of bug 1922315 ***