Bug 1497482

Hello,

 - %clean us not needed in Fedora. See https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections

%clean
rm -rf %{buildroot}

 - The changelog must contain the version and release info, and also your mail:

* Sat Sep 30 2017 Casey Deccio <casey> - 0.6.6-1


 - Remove the asterisks * at the end here:

%{python2_sitelib}/%{name}/*
%{python2_sitelib}/%{name}-%{version}-py2.7.egg-info/*
%{_datadir}/%{name}/*

otherwise these directories wouldn't be marked as owned.

 - You must own %{_defaultdocdir}/%{name} and %{_defaultdocdir}/%{name}/images too, add:

%dir %{_defaultdocdir}/%{name}
%dir %{_defaultdocdir}/%{name}/images


 - You've got Python scripts with shebang. See https://fedoraproject.org/wiki/Packaging_tricks#Remove_shebang_from_Python_libraries to remove them.

for lib in $(find %{buildroot}%{python2_sitelib}/%{name} -name '*.py'); do
 sed '1{\@^#!/usr/bin/env python@d}' $lib > $lib.new &&
 touch -r $lib $lib.new &&
 mv $lib.new $lib
done


Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


===== MUST items =====

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: "PSF (v2)", "GPL (v2 or later)", "Unknown or generated". 81
     files have unknown license. Detailed output of licensecheck in
     /home/bob/packaging/review/dnsviz/review-dnsviz/licensecheck.txt
[-]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/share/doc/dnsviz/images,
     /usr/share/dnsviz, /usr/lib/python2.7/site-packages/dnsviz-0.6.6-py2.7
     .egg-info, /usr/lib/python2.7/site-packages/dnsviz
[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/lib/python2.7/site-
     packages/dnsviz-0.6.6-py2.7.egg-info, /usr/lib/python2.7/site-
     packages/dnsviz, /usr/share/dnsviz, /usr/share/doc/dnsviz/images
[x]: Package contains no bundled libraries without FPC exception.
[!]: 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]: 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 839680 bytes in 59 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 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

Python:
[x]: Python eggs must not download any dependencies during the build
     process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep

===== SHOULD items =====

Generic:
[!]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: %clean present but not required
[-]: 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.
[-]: 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]: 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 all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: dnsviz-0.6.6-1.fc28.noarch.rpm
          dnsviz-0.6.6-1.fc28.src.rpm
dnsviz.noarch: E: wrong-script-interpreter /usr/lib/python2.7/site-packages/dnsviz/commands/graph.py /usr/bin/env python
dnsviz.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/dnsviz/commands/graph.py 644 /usr/bin/env python
dnsviz.noarch: E: wrong-script-interpreter /usr/lib/python2.7/site-packages/dnsviz/commands/grok.py /usr/bin/env python
dnsviz.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/dnsviz/commands/grok.py 644 /usr/bin/env python
dnsviz.noarch: E: wrong-script-interpreter /usr/lib/python2.7/site-packages/dnsviz/commands/print.py /usr/bin/env python
dnsviz.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/dnsviz/commands/print.py 644 /usr/bin/env python
dnsviz.noarch: E: wrong-script-interpreter /usr/lib/python2.7/site-packages/dnsviz/commands/probe.py /usr/bin/env python
dnsviz.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/dnsviz/commands/probe.py 644 /usr/bin/env python
dnsviz.noarch: E: wrong-script-interpreter /usr/lib/python2.7/site-packages/dnsviz/commands/query.py /usr/bin/env python
dnsviz.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/dnsviz/commands/query.py 644 /usr/bin/env python
dnsviz.src:38: W: macro-in-comment %check
dnsviz.src:39: W: macro-in-comment %{__python2}
2 packages and 0 specfiles checked; 10 errors, 2 warnings.

I've updated the spec to address your comments:

Spec URL: https://casey.byu.edu/dnsviz/dnsviz.spec

Please let me know what you think.

Cheers,
Casey

All ok, package accepted.

I've had to roll a 0.6.7 release with small but important changes to the code.  Do I use this same ticket and simply update the URLs for the spec file and SRPM?

The packaqe is accepted already, you can update it before import. No need to post anything anymore here.

(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/dnsviz

Hmm, how can the repo be created if cdeccio is not in the packager group?

Good question.

(In reply to Gwyn Ciesla from comment #8)
> Good question.

Indeed, reported at: https://pagure.io/fedscm-admin/issue/58

Hello Casey,

I just found your review has passed. I am interested in dns related products and would like to help this package finished.

However, you have not found someone to sponsor you into packager group [1]. I am not able to sponsor you myself, but I would like to help the package in the mean time. I find it curious dnsviz package exist, you are its owner but no build could have been done. Are you still interested maintaining this package? This bugzilla is not blocking FE-NEEDSPONSOR bug as it should.

I may help you becoming a package owner if you are still interested. Do you still want to become a Fedora package maintainer?

1. https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group#Convincing_someone_to_sponsor_you

Hi Petr,

Thanks so much for the comment.  In short, I would be delighted if DNSViz had an official Fedora package and maintainer, but I'm not sure that I have the cycles to be the maintainer myself--even though I was optimistic several years ago.  I just have too much going on.  And part of that is the overhead of getting sponsored and getting into the Fedora packager group.

What I've instead is written up details for installing on Fedora, using my best RPM knowledge :)
https://github.com/dnsviz/dnsviz/#fedora--rhel-8--centos-8-rpm-build-and-install

I hope that helps.

thanks,
Casey

It seems you are already owner of the package, which is interesting situation when you are unable to push the package. If you could add me on package source page [1] as admin or commiter, I would be able to finish the package and make it build. You would be able to fork and create Pull Requests even without being sponsored. Just someone of us packagers would have to review it and merge. I would gladly merge any change you would propose. Admin right is required to request stable fedora branch, commit is enough to push and built into existing branches.

We have also group of developers interested in DNS services under dns-sig group, which you might consider also adding with commit access. They include me as bind maintainer, knot maintainers and some similar. I am sure most of us used your dnsviz service not just once. Packaging it properly would be just small price to pay in return.

If you would be able to help with some pull requests, it should make sponsoring you even easier. It could work as a proof you understand RPM packaging enough to be a packager.

Would you able to give commit access to existing packagers? Especially as upstream maintainer it is desirable you have access to repo, you could be co-maintainer until we persuade someone to sponsor you [2]. Please consider giving the project in Settings tab if you know you do not have enough time to apply for sponsorship. dns-sig group access would be a minimal best choice.

1. https://src.fedoraproject.org/rpms/dnsviz/settings#usersgroups-tab
2. https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group#Become_a_co-maintainer

Just for information, there is updated spec file for python3 in upstream repository, which provides fine enough package. I think just build is needed.

Spec URL: https://github.com/dnsviz/dnsviz/raw/master/contrib/dnsviz.spec

Hi Petr,

Thanks for the much-needed direction for moving forward :)  I've added you as an admin and dns-sig as committers.  I'm happy to follow to help with pull requests for packaging, but I'll need your direction.

Yes, the spec file you referenced [1] seems to work well - I've used it to build Fedora, RHEL 8, CentOS 8 packages, as described in the README [2].  When I glanced at it again, I noticed that some tests could now be added [3,4].  Those tests were not built when the spec file was first written, hence my comment "#XXX no checks yet" :).

Let me know what next steps are.

Thanks,
Casey

[1] https://github.com/dnsviz/dnsviz/raw/master/contrib/dnsviz.spec
[2] https://github.com/dnsviz/dnsviz/#fedora--rhel-8--centos-8-rpm-build-and-install
[3] https://github.com/dnsviz/dnsviz/tree/master/tests
[4] https://github.com/dnsviz/dnsviz/blob/master/.travis.yml#L20

Great, Thank you!

Would you like to prepare initial spec commit by PR?

I have found small issue in existing spec. It includes only contents of directories %{python3_sitelib}/%{name}/*, %{python3_sitelib}/%{name}-%{version}-*.egg-info/* and %{_datadir}/%{name}/*.
It should include also the directory itself. Should be just:

%{python3_sitelib}/%{name}
%{python3_sitelib}/%{name}-%{version}-*.egg-info
%{_datadir}/%{name}

That is to prevent leaving there unowned directories. They can be checked by rpm -qf:

# rpm -qf /usr/share/dnsviz/
file /usr/share/dnsviz is not owned by any package

# rpm -qf /usr/share/dnsviz/css
dnsviz-0.9.3-1.fc36.noarch

Problem with those is when the package is uninstalled, empty unowned directories are not removed, but they should. Consider leaving just trailing slash at the end of name, but without asterisk.

As for tests, sure, I have noticed there are some. It would be great if they could be run during build. They must not require connectivity to the internet however. You can test self-contained builds by:

# fedpkg --release rawhide mockbuild

Which builds it the same way as production builders. Inside mock, without connectivity. They would also lack root access during build, only unprivileged ports are permitted. If those tests can run under such environment, they SHOULD be used.

Created attachment 1819055 [details]
Simple tests enablement patch

Thanks!  I'll try to get to these by the end of the week - or early next week.

FEDORA-2021-0969b85724 has been submitted as an update to Fedora 35. https://bodhi.fedoraproject.org/updates/FEDORA-2021-0969b85724

FEDORA-2021-3fb30be424 has been submitted as an update to Fedora 34. https://bodhi.fedoraproject.org/updates/FEDORA-2021-3fb30be424

FEDORA-2021-332ba102b5 has been submitted as an update to Fedora 33. https://bodhi.fedoraproject.org/updates/FEDORA-2021-332ba102b5

I had prepared some commits ready to build. I hope you spent no quite time on the same thing. I guess some version of dnsviz package is better than no build at all.

FEDORA-2021-0969b85724 has been pushed to the Fedora 35 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2021-0969b85724 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-0969b85724

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2021-3fb30be424 has been pushed to the Fedora 34 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2021-3fb30be424 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-3fb30be424

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2021-332ba102b5 has been pushed to the Fedora 33 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2021-332ba102b5 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-332ba102b5

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Actually, no, I did spend just a little time on it, but I hit a roadblock and ran out of time.  In short, no, I didn't spend much time :)

Thanks for your work!  What is next then?  Should I still just grab the spec file and update it as necessary in the upstream repo?  Or does that not make sense any more?

Yes, it would be nice to update spec file in upstream to match our spec. If other RPM distributions choose to package dnsviz, they should have available spec without known issues.

I think tests should be tweaked a bit to be easier to run. For example, if they had test_ prefix or _test.py suffix, they would be detected by pytest and all of them run automagically.
I made pull request with some changes required by pytest [1]. It does not solve failures on mockbuild, where no resolv.conf nameservers are available. It could not directly be used by the package without more tweaks, but I think pytest and %pytest macro would be useful for running tests once it is fixed.

I think about dnsviz.config, whether the current situation is sufficient. /usr/share/dnsviz/trusted-keys/root.txt seems like configuration file, which should not be overwritten on upgrade in case of user-made changes. It would be always overwritten without asking in current package state. Similar issue is with /usr/share/dnsviz/hints/named.root, if it needs to be modified. But that would require more code changes, probably move to /etc/dnsviz/named.root or similar path for config files.

It would be nice if dnsviz/config were made during normal build phase, so %py3_install macro could be used. I am not sure what would be better way however.

1. https://github.com/dnsviz/dnsviz/pull/87

FEDORA-2021-3fb30be424 has been pushed to the Fedora 34 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-2021-332ba102b5 has been pushed to the Fedora 33 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-2021-0969b85724 has been pushed to the Fedora 35 stable repository.
If problem still persists, please make note of it in this bug report.