Spec URL: https://mhayden.fedorapeople.org/asciigraph.spec SRPM URL: https://mhayden.fedorapeople.org/asciigraph-0.5.2-1.fc34.src.rpm Description: Go package to make lightweight ASCII line graph ╭┈╯ in command line apps with no other dependencies. Fedora Account System Username: mhayden
Hello! I am not a packager yet, so this is an unofficial review... > %files > %license %{golicenses} > %doc %{godocs} > %{_bindir}/* Would %{_bindir}/%{name} be preferable here? It looks like this package will try to own all files under %{_bindir}. > %changelog > %autochangelog This expands to * Tue Oct 19 2021 John Doe <packager> - 0.5.2-1.fc36 - local build The dist tag should not be in the changelog entry. The name/email does not match. ===== MUST items ===== [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 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 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 ===== [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]: 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: 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: asciigraph-0.5.2-1.fc36.x86_64.rpm golang-github-guptarohit-asciigraph-devel-0.5.2-1.fc36.noarch.rpm asciigraph-0.5.2-1.fc36.src.rpm asciigraph.x86_64: E: statically-linked-binary /usr/bin/asciigraph asciigraph.x86_64: W: no-manual-page-for-binary asciigraph golang-github-guptarohit-asciigraph-devel.noarch: W: hidden-file-or-dir /usr/share/gocode/src/github.com/guptarohit/asciigraph/.goipath asciigraph.src:62: W: macro-in-%changelog %autochangelog 3 packages and 0 specfiles checked; 1 errors, 3 warnings. --Robby
(In reply to Robby Callicotte from comment #1) > I am not a packager yet, so this is an unofficial review... Appreciated! > > %files > > %license %{golicenses} > > %doc %{godocs} > > %{_bindir}/* > > Would %{_bindir}/%{name} be preferable here? It looks like this package > will try to own all files under %{_bindir}. It will own only the ones that are created within this package at build time. I mainly do this to keep my spec files somewhat similar between packages. > > %changelog > > %autochangelog > > This expands to > * Tue Oct 19 2021 John Doe <packager> - 0.5.2-1.fc36 > - local build > > The dist tag should not be in the changelog entry. The name/email does not > match. Right. This depends on the package coming from a source git repo so that rpmautospec can generate the changelog based on the git history. Once it's in a source-git repo, this will be populated with the right data.
> > > %{_bindir}/* > > > > Would %{_bindir}/%{name} be preferable here? It looks like this package > > will try to own all files under %{_bindir}. > > It will own only the ones that are created within this package at build > time. I mainly do this to keep my spec files somewhat similar between > packages. I'd agree with Robby here. This is one of those case where a glob can easily mask errors: - as a reviewer, I don't know what binaries this package provides - during upgrades, if upstreams adds more binaries or renames the existing one, it is easy to miss Essentially, the names in /bin are the most important "api" of the package, and it's better to make this part explicit in the .spec file. > > > %changelog > > > %autochangelog > > > > This expands to > > * Tue Oct 19 2021 John Doe <packager> - 0.5.2-1.fc36 > > - local build > > > > The dist tag should not be in the changelog entry. The name/email does not > > match. > > Right. This depends on the package coming from a source git repo so that > rpmautospec can generate the changelog based on the git history. Once it's > in a source-git repo, this will be populated with the right data. Or in other words, our review tools haven't been updated yet to deal with rpmautospec. %changelog entries in fact *should* have dist tags, so the autogenerated changelog entry is correct. Unfortunately, this is a never-ending story: review tools and automatic checkers quite often trail behind the state-of-the-art packaging, and as a reviewer you need to learn and le-learn which parts of the output generated by review tools is obsolete. rpmlint: > asciigraph.x86_64: E: statically-linked-binary /usr/bin/asciigraph That's how go works, unfrotunately. > golang-github-guptarohit-asciigraph-devel.noarch: W: hidden-file-or-dir /usr/share/gocode/src/github.com/guptarohit/asciigraph/.goipath That's how go works, again. > asciigraph.src:62: W: macro-in-%changelog %autochangelog And that's rpmautospec, all fine. - package name is OK - latest version - license is acceptable for Fedora (BSD) - license is specified correctly - standard go template is used - builds and installs OK - BR/R/P look OK Package is APPROVED.
Thank you, Zbigniew! I'll change the glob to better reflect the names of the files there.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/asciigraph
In rawhide. https://koji.fedoraproject.org/koji/taskinfo?taskID=79403028