Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1338537
Summary: | Review Request: tarmux - Multiplex and demultiplex streams using tar file fragments. | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Graham Leggett <minfrin> |
Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> |
Status: | CLOSED NOTABUG | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | unspecified | ||
Version: | rawhide | CC: | innovation, ngompa13, nonamedotc, package-review |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | If docs needed, set a value | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2021-08-24 00:45:23 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 177841, 201449 |
Description
Graham Leggett
2016-05-22 14:40:22 UTC
Koji builds: http://koji.fedoraproject.org/koji/taskinfo?taskID=14215221 http://koji.fedoraproject.org/koji/taskinfo?taskID=14215243 More koji builds: http://koji.fedoraproject.org/koji/taskinfo?taskID=14215279 http://koji.fedoraproject.org/koji/taskinfo?taskID=14215321 Even before initiating a Fedora review, there are several problems with this spec. Aside from the fact that you published a spec.in file (which is not acceptable, you *must* publish the spec that is actually used in the source RPM), the packaging style is completely wrong. Here are the issues: * Defined BuildRoot and Packager. While BuildRoot doesn't hurt anything and is usually ignored, Packager is *never* supposed to be defined. That *must* be stripped out. %buildroot is automatically defined by RPM during build time. * Using overloaded macro variables to define fields. While this is not exactly not permitted, overloading the macro variables to define the fields is not. Please just put them into the fields themselves. It's redundant and pointless since the fields define those macro variables anyway. * Not using a path to sources. It makes life easier if the Source field is defined with a direct path to it. For example: "https://github.com/minfrin/%{name}/releases/download/%{name}-%{version}/%{name}-%{version}.tar.bz2" instead of just %{name}-%{version}.tar.bz2. * No usage of %{dist} in Release. Your release should be X%{?dist} instead of just X. For example, your Release field should be set to "1%{?dist}". See https://fedoraproject.org/wiki/Packaging:DistTag * Using Prefix. This is not permitted in Fedora at all. Please do not attempt to make a relocatable package. It causes too many weird things to happen. * Inadequate specification of build-time dependencies. Please specify all the components required to build (including compiler, etc.). We do not guarantee that a compiler will be present in the buildroot (even though currently one is always present). * Not using parallel make. Please use %make_build to specify parallel make build after running %configure. By default, Fedora prefers builds to be run with make -jX, and %make_build properly handles that for you. Run "rpm -E '%make_build'" to verify for yourself. * Redundant specification of parameters. The %configure macro already specifies all the things you're laying out there. Run "rpm -E '%configure'" to verify for yourself. * Empty scriptlet sections (%pre, %post, %preun). Please delete these as they add unnecessary bloat to the generated RPMs. * Redundant %clean section: Unless you're packaging for RHEL/CentOS 5, you don't need it, as RPM already has it pre-defined. * Redundant buildroot cleaning in %install phase and non-usage of %make_install macro. Again, unless you're targeting RHEL/CentOS 5, you don't need to clean the buildroot at the beginning of the %install section. As for the install, please use '%make_install'. It does the same thing, except it's cleaner. Run "rpm -E '%make_install'" to verify for yourself. * Usage of %attr in the file list. Do you really need this? Wouldn't the installation from your build scripts already set this? If not, perhaps you should fix it there. Usage of %attr is usually unnecessary if the build system can set it when make install is run. Please address all of these. Do you have an example of a "hello world" spec file with all of the above in place already? The spec file is very old and has been "handed down" from many projects. It would be far easier if there was a canonical spec file to use a base template to start again. There's an example on the Fedora Developer site: https://developer.fedoraproject.org/deployment/rpm/about.html I went through the spec file and made the changes you specified. Try this: Spec URL: https://github.com/minfrin/tarmux/blob/master/tarmux.spec > Inadequate specification of build-time dependencies. Please specify > all the components required to build (including compiler, etc.) Can you provide an example of how to do this? I've never encountered such a requirement before and am struggling to find examples, probably because the packages I've been building are based on other devel packages that provide the needed packages. Your new spec file looks much better now. It's always a good idea to run rpmlint against your newly built rpm files. Its output is not always correct, but it gives a hint where issues may turn up in the formal review. One point that is checked thoroughly in the review is the license of your files. You are using a standard license which has a short name of "ASL 2.0" according to https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#SoftwareLicenses So if you delete the "v" you should be good here. Your source file seems to be taken from a debian background, it contains many files that make sense only if you build a deb package. If possible, I recommend that you include only the few necessary files in the source code archive, because otherwise you'll end up with a number of files for which the review tool cannot assign a proper license. If you wish to see the results of an automatic package review for your package, install fedora-review and run "fedora-review -n tarmux" as a normal user in a directory where your spec file and SRPM exist. (You may have to add that user to the mock group.) (In reply to Graham Leggett from comment #6) > > Inadequate specification of build-time dependencies. Please specify > > all the components required to build (including compiler, etc.) > > Can you provide an example of how to do this? You can specify a number of BuildRequires. You'd probably need BuildRequires=gcc anyway. The automatic review tool will attempt to do a mock-build, which creates a minimal build environment based on your specification in the spec file. If the mock-build fails the build log shows you which requirements might not be satisfied. It won't actually complain about gcc, but if other libraries were needed to build you'd see it there. Just updated it to remove the v, and fedora-review seems happy. One of the details fedora-review complains about is that it cannot download the source, claiming 403 Forbidden. Works when I download it in a browser, seems to be github weirdness. tarmux.src: W: invalid-url Source0: https://github.com/minfrin/tarmux/releases/d ownload/tarmux-1.0.0/tarmux-1.0.0.tar.bz2 HTTP Error 403: Forbidden The debian files are for packaging for private debian repositories, and these files are ignored for the RPM packaging. The package will eventually be submitted to Debian, but Fedora first. (In reply to Graham Leggett from comment #9) > The debian files are for packaging for private debian repositories, and > these files are ignored for the RPM packaging. The package will eventually > be submitted to Debian, but Fedora first. But even debian don't expect these debian-specific files to be in the source tarball, as they are added during the debian packaging process. I suspect you're not familiar with the use of private yum/apt-get repositories for distributing software in the corporate world. The distros are not the only sources of packaged software, there is extensive use of private repos, and they need to keep their metadata somewhere. RPM is easy, a single spec file in the root of the package and all is done. Debian, being older, is more tricky, but the same pattern works. Bottom line is if you're a distro ignore the internal packaging and use your own. Just a few small changes before I start the formal review: 1) I understand that you intend to maintain a single source tarball that can be used to package in different distributions. At the moment this leads to 14 files with no known licenses. It is quite easy to resolve this as you can remove files from the builddirectory in the %prep macro before the software is compiled. Just change: %prep %setup -q rm -rf %{_builddir}/%{name}-%{version}/debian rm -rf %{_builddir}/%{name}-%{version}/any-other-unused-file 2) Please remove NEWS, because it is an empty file. 3) If you have officially build release 1, change the release to 2 and include the new spec file in a new SRPM. This is important, because at the moment your SRPM contains the old initial spec file. I recommend to keep both the new spec file and the SRPM up-to-date whenever you make any changes. And please update the changelog in the spec-file. Once this is done, I'll start the review. I created a new release to cover the changes, as they have been significant. Spec URL: https://github.com/minfrin/tarmux/blob/master/tarmux.spec SRPM URL: https://github.com/minfrin/tarmux/releases/download/tarmux-1.0.1/tarmux-1.0.1-1.fc23.src.rpm The changes have been made above, can you verify for me? Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. Note: These BR are not needed: gcc See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2 You can safely ignore this issue. It doesn't hurt to mention gcc. - Some files in the source tarball are reported as having no license: (see below) tarmux-1.0.1/AUTHORS tarmux-1.0.1/COPYING tarmux-1.0.1/ChangeLog tarmux-1.0.1/INSTALL tarmux-1.0.1/NEWS tarmux-1.0.1/README tarmux-1.0.1/compile tarmux-1.0.1/configure tarmux-1.0.1/depcomp tarmux-1.0.1/missing Most of them are clearly data files and don't need a license, but compile, configure, depcomp and missing are shell scripts (code), that need one. At a closer look, it turns out that all have a valid GPL license. So practically there is no need to remove any of them. If you wish to avoid their presence though, you can remove any of these files in %prep. This might be more important, if you have a more complex package than this one. So any changes here are cosmetic and do not block your package. ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [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: "MIT/X11 (BSD like)", "Apache (v2.0)", "Unknown or generated". 10 files have unknown license. Detailed output of licensecheck in /x/fedora/reviews/review-tarmux/licensecheck.txt see above: this is not a blocker. [-]: 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 [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. [x]: 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 10240 bytes in 3 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 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). [!]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in tarmux- debuginfo [x]: Package functions as described. Allthough the file permissions of tar file content are unusually open (666) [?]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: 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]: 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: tarmux-1.0.1-1.fc23.x86_64.rpm tarmux-debuginfo-1.0.1-1.fc23.x86_64.rpm tarmux-1.0.1-1.fc23.src.rpm tarmux.src: W: spelling-error Summary(en_US) demultiplex -> multiplex, multiple tarmux.src: W: spelling-error %description -l en_US demultiplex -> multiplex, multiple tarmux.src:10: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 10) tarmux.src: W: invalid-url Source0: https://github.com/minfrin/tarmux/releases/download/tarmux-1.0.1/tarmux-1.0.1.tar.bz2 HTTP Error 403: Forbidden This is not true, as wget works fine on this url. tarmux.x86_64: W: spelling-error Summary(en_US) demultiplex -> multiplex, multiple tarmux.x86_64: W: spelling-error %description -l en_US demultiplex -> multiplex, multiple 2 packages and 0 specfiles checked; 0 errors, 6 warnings. Rpmlint (debuginfo) ------------------- Checking: tarmux-debuginfo-1.0.1-1.fc23.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- sh: /usr/bin/python: No such file or directory tarmux-debuginfo.x86_64: E: no-changelogname-tag tarmux-debuginfo.x86_64: W: invalid-license ASL v2.0 tarmux.x86_64: E: no-changelogname-tag tarmux.x86_64: W: invalid-license ASL v2.0 tarmux.x86_64: W: empty-%pre tarmux.x86_64: W: empty-%post tarmux.x86_64: W: empty-%preun 2 packages and 0 specfiles checked; 2 errors, 5 warnings. This output reflects the old specfile! These errors are addressed in the new one. Requires -------- tarmux-debuginfo (rpmlib, GLIBC filtered): tarmux (rpmlib, GLIBC filtered): libarchive.so.13()(64bit) libc.so.6()(64bit) rtld(GNU_HASH) Provides -------- tarmux-debuginfo: tarmux-debuginfo tarmux-debuginfo(x86-64) tarmux: tarmux tarmux(x86-64) Source checksums ---------------- https://github.com/minfrin/tarmux/releases/download/tarmux-1.0.1/tarmux-1.0.1.tar.bz2 : CHECKSUM(SHA256) this package : 469a38e845d456ce75a99e55b7fb141a9e269faf0d37ddb26f8f5856bbd6e0d9 CHECKSUM(SHA256) upstream package : 469a38e845d456ce75a99e55b7fb141a9e269faf0d37ddb26f8f5856bbd6e0d9 Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02 Command line :/usr/bin/fedora-review -n tarmux Buildroot used: fedora-23-x86_64 Active plugins: Generic, Shell-api, C/C++ Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6 As none of the issues mentioned in the package review need addressing, your package "tarmux" is ***APPROVED*** Feel free to adapt any of my suggestions into your future spec file for that package. Unfortunately I'm not a sponsor, so you'll need to look for a person that is willing to sponsor you into the packager group. So you will need to submit another more complex package for review and you will need to show your understanding of the packaging guidelines to your sponsor by commenting on other people's review requests. As long as you have not been sponsored, you can comment on other package reviews (and you should do so once you are familiar with the guidelines) but you cannot perform a complete package review. I hope you'll find a sponsor soon, have a look at this group, feel free to contact anyone you know directly. https://admin.fedoraproject.org/accounts/group/members/packager/*/sponsor?_csrf_token=fa74806a4aef0d50d44894f07eab2f0e2340054f and https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group In order to attract attention, you should set a flag in one of your new package reviews FE-NEEDSPONSOR. Just enter this in the Blocks: field when you create a new bug for a review. All the best. Ralf First package, so requesting a sponsor. (In reply to Graham Leggett from comment #16) > First package, so requesting a sponsor. Are you still in need of a sponsor? I am, yes. Updated to the most recent v1.0.4 release: https://github.com/minfrin/tarmux/releases/tag/tarmux-1.0.4 https://github.com/minfrin/tarmux/releases/download/tarmux-1.0.4/tarmux-1.0.4-1.el8.src.rpm COPR build: https://copr.fedorainfracloud.org/coprs/minfrin/tarmux/build/1147957/ Most recent spec file: https://copr-be.cloud.fedoraproject.org/results/minfrin/tarmux/fedora-rawhide-x86_64/01147957-tarmux/tarmux.spec Review stalled, resetting status. If you need to find a sponsor consider opening a ticket on https://pagure.io/packager-sponsors/issues This is an automatic action taken by review-stats script. The ticket submitter failed to clear the NEEDINFO flag in a month. As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews we consider this ticket as DEADREVIEW and proceed to close it. |