Heya,

I'll review this one.

Could you review a few other tickets in the meantime also please? You can find pending ones here:

https://fedoraproject.org/PackageReviewStatus/

Cheers,
Ankur

A few notes already:

- We use the `fedora-review` tool for the first round of reviews, and that downloads the spec/srpm from the bug here, so we need to make sure that the links we provide here are ones that can be directly downloaded from:

Spec: https://download.copr.fedorainfracloud.org/results/niohiani/Diffuse-Python-3/fedora-rawhide-x86_64/01817739-diffuse/diffuse3-meson.spec
SRPM: https://copr-be.cloud.fedoraproject.org/results/niohiani/Diffuse-Python-3/srpm-builds/01817739/diffuse-0.6.0-60.fc33.src.rpm

- your spec file must match the name of the src.rpm (which both follow the naming guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/). So the spec should be called either python-diffuse or diffuse, depending on whether it's a library or only an application: and https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_naming

Looks pretty good. Here is a first round of reviews. A few blockers here that need fixing:

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

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


Issues:
=======
- Package uses either %{buildroot} or $RPM_BUILD_ROOT
  Note: Using both %{buildroot} and $RPM_BUILD_ROOT
  See: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_macros

- 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.
  Note: License file COPYING is not marked as %license
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/LicensingGuidelines/#_license_text

- Package does not use a name that already exists.
  Note: A package with this name already exists. Please check
  https://src.fedoraproject.org/rpms/diffuse
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/Naming/#_conflicting_package_names

^
This is OK.

- Spec file name must match the spec package %{name}, in the format
  %{name}.spec.
  Note: diffuse3-meson.spec should be diffuse.spec
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/#_spec_file_naming


- Why is the release 60? It can start with 1

- For the SourceURL, please refer to this page: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_hosting_services

- You do not need to BR: python3, python3-devel will pull it in.
- You do not need to BR: /usr/bin/python either.
- You do not need to Require: python3 either
- Are the explicit provides necessary?

- If this is a pure python package, it should be use: BuildArch: noarch, and then you do not need to explicitly disable the debug package. If not, it must generate debuginfo. https://docs.fedoraproject.org/en-US/packaging-guidelines/Debuginfo/

- We should use the metainfo file for appdata now: https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/
- Please also remember to validate the appdata file in the check section as listed there

- the %files tag is used twice, you can remove the first one.

===== 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: There is no build directory. Running licensecheck on vanilla
     upstream sources. Licenses found: "Unknown or generated", "GNU General
     Public License, Version 2", "*No copyright* GNU General Public
     License, Version 2". 101 files have unknown license. Detailed output
     of licensecheck in /home/asinha/dump/fedora-
     reviews/1923830-diffuse3-meson/licensecheck.txt

[!]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/share/diffuse
^
Perhaps the file list should have %{_datadir}/%{name} (remove the /*)?

[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/gnome/help,
     /usr/share/diffuse
^
I think you need to own the /usr/share/gnome/help directory (while you do not need to Require a package for it). Take a look here:
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_the_directory_is_owned_by_a_package_which_is_not_required_for_your_package_to_function

[!]: %build honors applicable compiler flags or justifies otherwise.
^
Is nothing being built in the build step? You can remove %meson_build if that's the case.

[x]: Package contains no bundled libraries without FPC exception.
[!]: Changelog in prescribed format.
^
Please correct the changelog. It should be version-release at the end, and the entries are generally short points. You can give more verbose explanations in the git commit messages when committing to Fedora SCM

https://docs.fedoraproject.org/en-US/packaging-guidelines/#changelogs


[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: The spec file handles locales properly.
[!]: Package consistently uses macros (instead of hard-coded directory
     names).
^
Please use %{buildroot} consistently (and not mix $RPM_BUILD_ROOT too).

[!]: Package is named according to the Package Naming Guidelines.
^
Please see my comment above.

[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.
^
Please see my comment on the extra Requires.

[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[!]: Useful -debuginfo package or justification otherwise.
^
Please see my comment above.

[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 40960 bytes in 4 files.
^
Should the help bits be moved to a -doc sub-package?

[!]: Package complies to the Packaging Guidelines
^
A few things to work on here.

[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]: Package does not own files or directories owned by other packages.
[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]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install or
     desktop-file-validate if there is such a file.
[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]: No %config files under /usr.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[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.
[!]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
^
Not tested this out.

[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.
^
Not tested this out, we'll do scratch builds on koji later to confirm.

[-]: %check is present and all tests pass.
^
There are no tests from the looks of it.

[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:
[!]: Spec file according to URL is the same as in SRPM.
     Note: Bad spec filename: /home/asinha/dump/fedora-
     reviews/1923830-diffuse3-meson/srpm-unpacked/diffuse3-meson.spec
     See: (this test has no URL)
[!]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
     Note: Arch-ed rpms have a total of 1003520 bytes in /usr/share
^
It is possible to split the package into sub-packages here, but let's look at that after the first round of tweaks.

[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).


Rpmlint
-------
Checking: diffuse-0.6.0-60.fc34.x86_64.rpm
          diffuse-0.6.0-60.fc34.src.rpm
diffuse.x86_64: W: incoherent-version-in-changelog 0.6.0 ['0.6.0-60.fc34', '0.6.0-60']
diffuse.x86_64: E: no-binary
diffuse.x86_64: E: incorrect-fsf-address /usr/share/doc/diffuse/COPYING
diffuse.src: E: invalid-spec-name
diffuse.src:22: W: unversioned-explicit-provides mergetool
diffuse.src:23: W: unversioned-explicit-provides difftool
diffuse.src:8: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 8)
diffuse.src: E: specfile-error warning: line 90: multiple %files for package 'diffuse'
2 packages and 0 specfiles checked; 4 errors, 4 warnings.


^
These are covered above.


Rpmlint (installed packages)
----------------------------
diffuse.x86_64: W: incoherent-version-in-changelog 0.6.0 ['0.6.0-60.fc34', '0.6.0-60']
^
Please correct.

diffuse.x86_64: E: no-binary
^
I expect this is because the package should be a noarch package.

diffuse.x86_64: E: incorrect-fsf-address /usr/share/doc/diffuse/COPYING
1 packages and 0 specfiles checked; 2 errors, 1 warnings.

^
Please report the incorrect fsf address to upstream

Source checksums
----------------
https://codeload.github.com/MightyCreak/diffuse/tar.gz/v0.6.0 :
  CHECKSUM(SHA256) this package     : 806ad7e4efd6408078d4667d763bf8efcd5a01bd152b82f38450539e3e0ec74d
  CHECKSUM(SHA256) upstream package : 806ad7e4efd6408078d4667d763bf8efcd5a01bd152b82f38450539e3e0ec74d


Requires
--------
diffuse (rpmlib, GLIBC filtered):
    /usr/bin/python3
    config(diffuse)
    hicolor-icon-theme
    python3



Provides
--------
diffuse:
    application()
    application(diffuse.desktop)
    config(diffuse)
    difftool
    diffuse
    diffuse(x86-64)
    mergetool
    metainfo()
    metainfo(diffuse.appdata.xml)
    mimehandler(text/plain)
    mimehandler(text/x-chdr)
    mimehandler(text/x-csrc)



Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10
Command line :/usr/bin/fedora-review -b 1923830
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api
Disabled plugins: PHP, Python, R, Haskell, fonts, Perl, Ocaml, C/C++, Java, SugarActivity
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

Thank you for all of the useful input. I'll look over all of this and start making the necessary changes. I'm pretty sure I can slim down the Build Requirements in the spec too. It might take a few days for me to have *everything* remedied, as I have to do some pretty heavy travelling this week, but I will get it done.

Starting to make changes now! Sorry for the delay. Long trip. Will share a modified spec file when I think I have everything in order. Thanks again!

Hey again folks! Just an update.

The spec file I'm amending to meet the criteria for inclusion is here now: https://gist.github.com/bongochong/2caa25a6121487a270ac406f2e052921

I think I've addressed all - or at least most - of the issues, but if I notice anything else (or you notice anything else) that needs changing, just give me a heads up. Waiting for Ankur's feedback in particular. Thanks for being so thorough and communicative.

Hello,

Sorry, missed the notification here. Could you please post the URLs for both the spec/srpm so that I can run it through fedora-review again?

Cheers,
Ankur

(In reply to Ankur Sinha (FranciscoD) from comment #7)
> Hello,
> 
> Sorry, missed the notification here. Could you please post the URLs for both
> the spec/srpm so that I can run it through fedora-review again?
> 
> Cheers,
> Ankur

Sorry, it has been an insane past few weeks, but here you go!
Spec: https://gist.github.com/bongochong/2caa25a6121487a270ac406f2e052921
SRPM: https://download.copr.fedorainfracloud.org/results/niohiani/Diffuse-Python-3/fedora-rawhide-x86_64/01963703-diffuse/diffuse-0.6.0-61.fc35.src.rpm

After another round of feedback I'm sure I'll get it down pat. Apologies for the delay.

Hrm, fedora-review still isn't finding it.

Let's try again:


Spec: https://gist.githubusercontent.com/bongochong/2caa25a6121487a270ac406f2e052921/raw/e7fd871f9dc26c77c9d0844b06da9af969ff90f6/diffuse.spec
SRPM: https://download.copr.fedorainfracloud.org/results/niohiani/Diffuse-Python-3/fedora-rawhide-x86_64/01963703-diffuse/diffuse-0.6.0-61.fc35.src.rpm

Almost there, a little more work needed :)

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

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


Issues:
=======
- Package uses either %{buildroot} or $RPM_BUILD_ROOT
  Note: Using both %{buildroot} and $RPM_BUILD_ROOT
  See: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_macros

^
Please use one form, for consistency.

- 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.
  Note: License file COPYING is not marked as %license
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/LicensingGuidelines/#_license_text
^
Please mark the license file using %license.

- Package does not use a name that already exists.
  Note: A package with this name already exists. Please check
  https://src.fedoraproject.org/rpms/diffuse
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/Naming/#_conflicting_package_names

^
This is OK, since this is a re-review


- Directory ownership issues, please see below.

- Please prefer this form for the Source etc: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags

- please do not mix tabs and space: best to only use one form consistently

- if it's a noarch package, you do not need to disable debuginfo. Please remove the line

- the appdata files now go to the metainfodir etc. Please see this: https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/

- please correct the older changelog entries

- your spec/srpm don't match: the srpm version is 0.6.0-61? It should be 0.6.0-1?


===== 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: "Unknown or generated", "GNU General Public License, Version
     2", "*No copyright* GNU General Public License, Version 2". 101 files
     have unknown license. Detailed output of licensecheck in
     /home/asinha/dump/fedora-reviews/1923830-diffuse/licensecheck.txt

[!]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/share/gnome/help/diffuse/it,
     /usr/share/gnome/help/diffuse/ru, /usr/share/omf/diffuse,
     /usr/share/gnome/help/diffuse, /usr/share/gnome/help/diffuse/C,
     /usr/share/gnome/help/diffuse/cs

^ Please check these directories for ownership. Nothing seems to own /usr/share/gnome/help, so this package should own it.
Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_and_directory_ownership

Since the help files use gnome-help

[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/gnome/help/diffuse,
     /usr/share/gnome/help/diffuse/ru, /usr/share/gnome/help/diffuse/C,
     /usr/share/gnome/help, /usr/share/gnome/help/diffuse/cs,
     /usr/share/omf/diffuse, /usr/share/gnome/help/diffuse/it

^
See point above.

[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
^

Looks fine. I'd add a blank like after each changelog entry for readability.

[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: The spec file handles locales properly.
[!]: Package consistently uses macros (instead of hard-coded directory
     names).

^
You can use %{name} in the files section etc. too

[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.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 40960 bytes in 4 files.
[!]: Package complies to the Packaging Guidelines
^
Some work needed :)

[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]: Package does not own files or directories owned by other packages.
[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]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install or
     desktop-file-validate if there is such a file.
[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]: No %config files under /usr.
[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).
[?]: Package functions as described.
^
Not tested the functioning out.

[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.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
^
No tests are defined in the package

[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:
[!]: 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 all installed packages.
     Note: There are rpmlint messages (see attachment).


Rpmlint
-------
Checking: diffuse-0.6.0-61.fc35.noarch.rpm
          diffuse-0.6.0-61.fc35.src.rpm
diffuse.noarch: W: incoherent-version-in-changelog 0.6.0 ['0.6.0-61.fc35', '0.6.0-61']
^
Please ensure that the srpm is generated from the correct version of this spec

diffuse.noarch: E: incorrect-fsf-address /usr/share/doc/diffuse/COPYING
^
Please report this upstream

diffuse.src:18: W: unversioned-explicit-provides mergetool
diffuse.src:19: W: unversioned-explicit-provides difftool
diffuse.src:80: W: mixed-use-of-spaces-and-tabs (spaces: line 80, tab: line 1)
2 packages and 0 specfiles checked; 1 errors, 4 warnings.




Rpmlint (installed packages)
----------------------------
diffuse.noarch: W: incoherent-version-in-changelog 0.6.0 ['0.6.0-61.fc35', '0.6.0-61']
diffuse.noarch: E: incorrect-fsf-address /usr/share/doc/diffuse/COPYING
1 packages and 0 specfiles checked; 1 errors, 1 warnings.
^
Please report this upstream



Source checksums
----------------
https://codeload.github.com/MightyCreak/diffuse/tar.gz/v0.6.0 :
  CHECKSUM(SHA256) this package     : 806ad7e4efd6408078d4667d763bf8efcd5a01bd152b82f38450539e3e0ec74d
  CHECKSUM(SHA256) upstream package : 806ad7e4efd6408078d4667d763bf8efcd5a01bd152b82f38450539e3e0ec74d


Requires
--------
diffuse (rpmlib, GLIBC filtered):
    /usr/bin/python3
    config(diffuse)
    hicolor-icon-theme



Provides
--------
diffuse:
    application()
    application(diffuse.desktop)
    config(diffuse)
    difftool
    diffuse
    mergetool
    metainfo()
    metainfo(diffuse.appdata.xml)
    mimehandler(text/plain)
    mimehandler(text/x-chdr)
    mimehandler(text/x-csrc)



Diff spec file in url and in SRPM
---------------------------------
--- /home/asinha/dump/fedora-reviews/1923830-diffuse/srpm/diffuse.spec	2021-04-21 19:45:07.621795900 +0100
+++ /home/asinha/dump/fedora-reviews/1923830-diffuse/srpm-unpacked/diffuse.spec	2021-02-13 01:54:02.000000000 +0000
@@ -1,5 +1,5 @@
 Name:			diffuse
 Version:		0.6.0
-Release:		1%{?dist}
+Release:		61%{?dist}
 Summary:		Graphical tool for merging and comparing text files
 License:		GPLv2+
@@ -115,10 +115,10 @@
         </p>
         <p>
-          The next version will not take 6 years, I promise you!
+          The next version will not take 6 years, I promise you! 😄
         </p>
         <p>Added:</p>
         <ul>
           <li>Added Pedro Albuquerque's Portuguese translation</li>
-          <li>Added Åke Engelbrektson's Swedish translation</li>
+          <li>Added Ã…ke Engelbrektson's Swedish translation</li>
           <li>Added Guillaume Hoffmann's Darcs support improvements</li>
           <li>Added Akom Chotiphantawanon's Thai translation</li>
@@ -177,8 +177,8 @@
 
 %changelog
-* Fri Feb 12 2021 bongochong <bongochong> 0.6.0-1
+* Fri Feb 12 2021 bongochong <bongochong> Version 0.6.0
 - Working on bringing packaging of this application up to par for inclusion in default repos
-* Mon Dec  7 2020 bongochong <bongochong> 0.6.0
+* Mon Dec  7 2020 bongochong <bongochong> Version 0.6.0
 - Updated to 0.6.0. Mainly under the hood changes in this release, so nothing really visible to the users in this version. That said, I figured it was a long time since the last release (4 months ago) and, as promised, I want Diffuse development to be a bit more active and iterative. Replace old install.py with the more standard Meson. Remove u string prefixes since Python 3 is in UTF-8 by default. Replaced some interpolation operators (%) for the f string prefix. Use the window scale factor for the icons generation
-* Tue Nov  3 2020 bongochong <bongochong> 0.5.9
+* Tue Nov  3 2020 bongochong <bongochong> Version 0.5.9
 - Fedora Packaging of Python 3 Fork and Initial upload to COPR of said fork
\ No newline at end of file


Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10
Command line :/usr/bin/fedora-review -b 1923830
Buildroot used: fedora-rawhide-x86_64
Active plugins: Shell-api, Generic
Disabled plugins: SugarActivity, Haskell, Perl, fonts, R, Ocaml, Python, C/C++, PHP, Java
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

Thanks again, again, Ankur! I'll get on this very soon, and after this round of corrections it should all be in order. I'll create a temporary dedicated COPR repo just for getting the packaging up to snuff, so there's no conflicts for myself and the handful of others who use my repos.

Alright Ankur! I think I've taken care of all the "Musts" by now, along with the other things you pointed out. I did my best to address directory/file ownership via the %ghost directive (which I just learned about); Increased the use of macros as opposed to explicit paths; Marked the license file; Consistently used %{buildroot} instead of $RPM_BUILD_ROOT; Edited the changelog entries for readability. The dedicated repo has also allowed me to remedy the versioning issue.

New testing repo for packaging: https://copr.fedorainfracloud.org/coprs/niohiani/Diffuse-Packaging-Test/

Spec: https://download.copr.fedorainfracloud.org/results/niohiani/Diffuse-Packaging-Test/fedora-rawhide-x86_64/02159737-diffuse/diffuse.spec

SRPM: https://download.copr.fedorainfracloud.org/results/niohiani/Diffuse-Packaging-Test/fedora-rawhide-x86_64/02159737-diffuse/diffuse-0.6.0-1.fc35.src.rpm

If I've missed anything, I'll of course be happy to continue amending the .spec file. Thanks so much for your patience.

Heya,

Looks pretty good. Will give it a full review again this week. Some tweaks that I noticed that got left behind:

- please remove the "%global debug_package %{nil}" line. If the package is noarch, it will not generate debuginfo. If it isn't noarch, it *must* generate and include debuginfo.

- I don't think this is the right usage for the %ghost macro. As the docs say, files that are generated by the package at runtime should be marked as %ghost. The files here are not so---they are included in the rpm. Since no other package in the Fedora repositories owns these directories, this package should just own these  directories, so please remove the %ghost directive there. (Docs: http://ftp.rpm.org/max-rpm/s1-rpm-inside-files-list-directives.html) 

- the appdata files now go to the metainfodir etc. Please see this: https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/

- Also, just to double check: is the e-mail you are using in the change log correct, since it does not match your e-mail here on bugzilla? (You should use the e-mail you use here in Bugzilla, which should match the e-mail you provide in the Fedora accounts system).

- since COPYING is marked by the %license tag, you do not need to repeat it in the %doc list again.

Cheers,
Ankur

Shoot. I thought I caught everything. Sorry for missing the debug_package line. I'll fix that. It's definitely noarch. I must have misunderstood what I read about %ghost, so I'll remedy that too. I admit that I got a bit confused about specifying directory ownership in the spec file, but after a little more reading I think I've got it (such directories should be present under %files), but please correct me if I'm wrong. I'll do some more reading about the AppData topic, and I'll remove the COPYING file from the %doc list. I'll amend the spec file and do another build on COPR in the next 24 hours, so forget about reviewing the spec and SRPM in the previous comment, as a new one is incoming. Thanks!

OK, I jumped on this as quick as I could, and addressed the aforementioned issues, along with switching to the eMail address associated with my user account in the changelog.

Spec: https://download.copr.fedorainfracloud.org/results/niohiani/Diffuse-Packaging-Test/fedora-rawhide-x86_64/02161033-diffuse/diffuse.spec

SRPM: https://download.copr.fedorainfracloud.org/results/niohiani/Diffuse-Packaging-Test/fedora-rawhide-x86_64/02161033-diffuse/diffuse-0.6.0-1.fc35.src.rpm

I'm not 100% sure about the validity of the method I used to specify that diffuse owns the directories you mentioned (placing them under the %files directive), so if there's something I should be doing differently in that regard, just let me know what it is and I'll make the necessary modifications. Also, I based the transition of the AppData files into the metainfo directory on commits I saw in other projects that needed to do the same (I learn best through direct examples), so if there is a better way to do it, I'm glad to make those modifications with the necessary instructions as well.

P.S. I noticed - because I always have the following program installed under any distro I use - that GChemTable also creates a directory within /usr/share/gnome/help under Fedora, so I did not list the entire /usr/share/gnome/help directory under %files in my spec. I did list all the other ones you mentioned though.

Sorry for the delay---got very busy all of a sudden.

OK, that looks good! There are one or two items remaining, but you can do those before you import the package.

- in the files section, you only need the one line (since it implies that it also owns all sub-directories):

%{_datadir}/gnome/help/%{name}

instead of 

%{_datadir}/gnome/help/%{name}
%{_datadir}/gnome/help/%{name}/C
%{_datadir}/gnome/help/%{name}/cs
%{_datadir}/gnome/help/%{name}/it
%{_datadir}/gnome/help/%{name}/ru


- rpmlint shows that the FSF address in the license file is wrong (it uses the old address and needs to be updated)

diffuse.noarch: E: incorrect-fsf-address /usr/share/licenses/diffuse/COPYING

please file a ticket upstream to ask them to update this (please do not update the address yourself, since we must use the license file in the form that upstream provides it).

XXX APPROVED XXX

With regards to sponsorship to the packagers group, could you review one or two tickets please? Reviewing packages for colleagues is as much a duty of us package maintainers as maintaining our own packages is. So you also need to know how to do that. You can use `fedora-review` to help you.

I guess you continue from this bit:

https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner

I'll be happy to help with your reviews too, of course. Please feel free to CC me to any tickets and ask me any questions on them (or directly over e-mail at ankursinha AT fedoraproject.org)
.
You can find all tickets awaiting review here:

https://fedoraproject.org/PackageReviewStatus/

Please note in your reviews that you are awaiting sponsorship (since you cannot approve packages until you are sponsored).

Cheers,
Ankur

Many thanks Ankur!

I'll make a pull request in MightyCreak's repo to correct the address in the license file, and of-course reduce the relevant entries under %files to that one line. I'll get into pitching in with those package reviews ASAP as well.

Final revision of spec file is here: https://download.copr.fedorainfracloud.org/results/niohiani/Diffuse-Packaging-Test/fedora-rawhide-x86_64/02210632-diffuse/diffuse.spec

SRPM is here: https://download.copr.fedorainfracloud.org/results/niohiani/Diffuse-Packaging-Test/fedora-rawhide-x86_64/02210632-diffuse/diffuse-0.6.0-1.fc35.src.rpm

Pull request in the Diffuse repository for correcting the FSF mailing address is here: https://github.com/MightyCreak/diffuse/pull/97/commits/4abb8be85b5bd2f769db632b93a2f5252424ca89

I will continue on with importing Diffuse according to the link you posted, but want to make sure I have the time to do it all correctly, so it might take a few days (I'm traveling tomorrow morning). I will also start pitching in with package reviews as well, but that might take a few days too, as I'll be with my travel laptop, and feel like I should pick simpler packages so I can actually be useful. Thanks so much again Ankur, and I shall be in touch very soon.

P.S. The package testing COPR repo will be removed once I complete the aforementioned tasks.

P.P.S. I had to travel for a bit longer than expected due to a family situation, but am back home now and will get on top of everything over the next few days. Sincere apologies for the delay.