This is not a full review, just a few things I saw at first glance.

There are several bundled libraries in src/3rdParty. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling. In short, if upstream supports building with an external copy, you must do so. Otherwise, you must contact upstream requesting the ability to do so; then you may either add a virtual Provides for the bundled library or, if you like, find a way to patch it to unbundle yourself.

A number of files have copyright headers indicating they have licenses other than MIT. All licenses from files that contribute to the final build need to be in the License field, with some documentation of which license applies to which parts. See https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_multiple_licensing_scenarios. You can use licensecheck to help look for files that you need to examine.

You must validate the AppData XML file in %check; see https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/#_app_data_validate_usage.

(In reply to code from comment #1)
> This is not a full review, just a few things I saw at first glance.

Thank you, I'm working on it.

(In reply to code from comment #1)

Licences and desktop files fixed.

SPEC: https://tieugene.fedorapeople.org/rpms/qvge/qvge.spec
SRPM: https://tieugene.fedorapeople.org/rpms/qvge/qvge-0.6.2-2.20210114gitfdcf075.fc33.src.rpm
Koji builds:
- http://koji.fedoraproject.org/koji/taskinfo?taskID=60023254
- http://koji.fedoraproject.org/koji/taskinfo?taskID=60024354
- http://koji.fedoraproject.org/koji/taskinfo?taskID=60024974
- http://koji.fedoraproject.org/koji/taskinfo?taskID=60025446

As for src/3rdParty/ - there are parts of old, forgotten projects compiled-in statically.
And I think there is no sense to package them separately.

> As for src/3rdParty/ - there are parts of old, forgotten projects compiled-in statically.
> And I think there is no sense to package them separately.

You still need to follow the guidelines in https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling, regardless of the age, upstream support, or popularity of the bundled dependencies.

1. If upstream allows building against an external library without patching, you MUST do so. That means you MUST package them separately, however inconvenient that may be. I’m not intimately familiar with the Qt build system, but it looks like that is not the case.

2. If upstream does not allow building against an external library without patching, people often try to develop a patch, since bundled libraries still have disadvantages (https://fedoraproject.org/wiki/Bundled_Libraries?rd=Packaging:Bundled_Libraries). You have made a reasonable argument against putting much effort into that. Again, I’m not intimately familiar with the Qt build system, but it looks like a patch would probably be nearly trivial, as the config files are simple text files and there are already examples of using external libraries.

3. If upstream does not allow building against an external library without patching, then you can choose to bundle. This is what you say you would prefer. In addition to dealing with the bundled library licenses, you must add virtual Provides with appropriate names and, if possible, versions, for all bundled libraries; you must publicly contact upstream about a path to supporting system libraries (see https://github.com/ArsMasiuk/qvge/issues/109, about Boost, which used to be bundled); and if upstream refuses, you must record it in a comment or in a source file referenced from a comment adjacent to the virtual Provides. All of these are MUST, not SHOULD. For the exact requirements, read the linked guidelines page.

For reference, these seem to be the upstreams for the bundled libraries in src/3rdParty/:

qprocessinfo:      https://github.com/baldurk/qprocessinfo/
QSint:             https://sourceforge.net/projects/qsint/
QtPropertyBrowser: https://github.com/abhijitkundu/QtPropertyBrowser/

Do you still intend to complete this package? I can do the review — unless Ben wants to, it seems like he is reviewing, but the fedora-review tag has not been set. The bundled dependencies issue still needs to be resolved.

I was prepared to set the fedora-review flag and start the review if the submitter resolved the initial issues—but I am also just as happy for someone else to pick it up.

So two possible reviewers, great! Let us say, in case Eugene is still interested and resolves the initial issues, the first to set the flag gets to review.

(In reply to Otto Urpelainen from comment #6)
> Do you still intend to complete this package? I can do the review — unless
> Ben wants to, it seems like he is reviewing, but the fedora-review tag has
> not been set. The bundled dependencies issue still needs to be resolved.

The problem is in 3rd-parties/*:
- qprocessinfo (https://github.com/baldurk/qprocessinfo)
- qsint-widgets (part of https://sourceforge.net/projects/qsint)
- qtpropertybrowser (part of https://github.com/qtproject/qt-solutions)
These are old, mostly orphaned libraries. But they are 3rd-parties.
Solitions:
1. Make these things as new packages then push qvga using them as system libs. It is really hard because requires many-many patches without developers support. And at the end of ends nobody needs them.
2. Make them as bundled libs and install as extra libs in qvge.spec. As qvge linked statically with them it is a bulk of job too to remake compiling and installation scripts. After this we will have those 3rdparties as separate libs installing during qvge installation. And nobody needs them after this too.
But Fedora packaging policy requires to choose only one from 2 above.

I don't know what to do with this.

Please re-read the guidelines around bundled dependencies carefully; I think you have some misunderstandings.

When you bundle dependencies, Fedora doesn’t require you to install the bundled dependencies as “extra libs”, i.e., separate shared libraries. It requires the following:

  1. Upstream does not support building with an external copy of the dependency. If it did, you would NOT be permitted to bundle, no matter how inconvenient the extra packaging might be.

  2. You have publicly contacted upstream about a path to supporting system libraries; and if upstream refused, recorded it in a comment or in a source file referenced from a comment adjacent to the virtual Provides.

  3. You have added the appropriate virtual Provides to your spec file. These are just metadata, which allow bundling to be tracked.

Having followed these steps, you are NOT required to patch the build system in any way.

I know dealing with bundled dependencies is a perennial pain, but you have to fully understand the guidelines around them and pick one of the permissible options.

(In reply to Ben Beasley from comment #10)

Ok, let's go:

>   1. Upstream does not support building with an external copy of the
> dependency. If it did, you would NOT be permitted to bundle, no matter how
> inconvenient the extra packaging might be.

This was not discussed because Fedora/EPEL repos have no appropriate packages and I have no plan to package them (see my note №1)

>   2. You have publicly contacted upstream about a path to supporting system
> libraries; and if upstream refused, recorded it in a comment or in a source
> file referenced from a comment adjacent to the virtual Provides.

There are no such system libs, see above.

>   3. You have added the appropriate virtual Provides to your spec file.
> These are just metadata, which allow bundling to be tracked.

qvge *not* installs any library but statically compiled with them in-place.
Just one binary and nothing else.
So I cannot provide anything but qvge itself.

> This was not discussed because Fedora/EPEL repos have no appropriate packages and I have no plan to package them (see my note №1)

There’s no need to argue with me; I didn’t write the guidelines. Sometimes the upstream contact feels like a formality, with little hope of any change, but it is unambiguously required: https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling. If you don’t plan to unbundle, this route is your only option. It’s not hard to do.

> There are no such system libs, see above.

Right. If upstream supported building with external dependencies, you would have to package them. Since it doesn’t, you don’t have to; but you have to follow https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling.

> qvge *not* installs any library but statically compiled with them in-place.
> Just one binary and nothing else.
> So I cannot provide anything but qvge itself.

The guidelines explain this, and I did too. The *virtual* Provides just allows tracking bundled libraries. It does not require you to offer a usable copy of the library, to build it separately, or anything else. That’s why you are providing “bundled(foo) = ${bundled-version}”, or just “bundled(foo)” if the bundled library is unversioned, not providing “foo” itself.

----

The guidelines around this are only a screenful. The process is simpler than it used to be and no longer requires petitioning the FPC. You’re going to have to slow down and take the time to understand your options. Having to deal with bundled dependencies is normal, and I don’t think you’re going to find a reviewer who decides you don’t have to do it the same way other packagers have to.

----

I think I’m done commenting here unless there’s a new, concrete question I can answer, or an updated submission for review.

(In reply to Ben Beasley from comment #12)
> I think I’m done commenting here unless there’s a new, concrete question I
> can answer, or an updated submission for review.

Thank you for advices.
As I never made packages with bundling libs I tried to make as understand guidelines and your advices.
Spec: https://tieugene.fedorapeople.org/rpms/qvge/qvge.spec
SRPM: https://tieugene.fedorapeople.org/rpms/qvge/qvge-0.6.2-3.20210405git0fd4648.fc34.x86_64.rpm
Koji F34: https://koji.fedoraproject.org/koji/taskinfo?taskID=67163521

Thank you Eugene, it is looking better!

There is a small mistake, the file you link to is not srpm but binary rpm. I got the srpm from your Koji build to get fedora-review running. More comments to follow soon.

I have now made a complete review. Findings:

1. I think the License should be "(MIT and LGPLv3 and BSD)". The licensing guidelines are not crystal clear on this, but I read them as a) no parenthesis: multiple files, each with one of the listed licenses, b) with parenthesis: one file containing parts with each of the licenses.

2. All these licenses require including a copy of the license when distributing the code. So, either the LICENSE file needs to be updated to contain licenses for everything that is bundled, or some other way invented to get the all the licenses and copyright notices included in %license. I think a pull request is needed upstream to get this right.

3. Annoyingly, AppStream metadata also contains a field for liceses, project_license. So qvge.appdata.xml must also be updated to list the actual license that has now been determined. That goes naturally to the same pull request where licensing is otherwise made compatible with Fedora packaging.

4. Add "BuildRequires: make". This should be explictly listed nowadays if make is used.

5. fedora-review complains: "Note: Directories without known owners: /usr/share/mime/packages, /usr/share/mime". Add directories must have assigned ownership, in this case the correct solution would be to add "Requires: shared-mime-info", which is a "filesystem package" owning those directories. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_and_directory_ownership

6. There is no man page, you should get in contact with the upstream about adding such. Pull request is best, but an issue will also do as usual. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages

7. Since running as many tests as possible in %check is recommended, perhaps add a comment line there explaining that upstream does not provide any? Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_test_suites

I am confident in my review, but if Ben still have some comments, they are welcome of course.

Thanks for taking the review.

-----

> I think the License should be "(MIT and LGPLv3 and BSD)". The licensing guidelines are not crystal clear on this, but I read them as a) no parenthesis: multiple files, each with one of the listed licenses, b) with parenthesis: one file containing parts with each of the licenses.

See https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_multiple_licensing_scenarios, which has some examples; and https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_combined_dual_and_multiple_licensing_scenario and https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_mixed_source_licensing_scenario, which show some scenarios where parentheses are needed for grouping.

It’s my understanding that parentheses around the entire license expression are valid but have no particular meaning, and that the distinction between the cases (a) and (b) is handled by the required comment explaining the multiple licensing breakdown.

-----

> All these licenses require including a copy of the license when distributing the code.

Great catch! I’ll just add a helpful link to the relevant Guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text

-----

> Annoyingly, AppStream metadata also contains a field for liceses, project_license. So qvge.appdata.xml must also be updated to list the actual license that has now been determined.

Does Fedora require this? Or is it adequate for the metadata to hold the “overall” upstream project license, eliding other compatible licenses for various bits of the source, as is relatively common practice outside of Fedora RPM packaging? I honestly don’t know the answer to this.

-----

> There is no man page, you should get in contact with the upstream about adding such. Pull request is best, but an issue will also do as usual. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages

Agreed, although I will note that this is a SHOULD rather than a MUST, and that help2man can be useful sometimes.

(In reply to Ben Beasley from comment #16 and Otto Urpelainen from comment #15)

Ok, let's go next level:
Spec: https://tieugene.fedorapeople.org/rpms/qvge/qvge.spec
SRPM: https://tieugene.fedorapeople.org/rpms/qvge/qvge-0.6.2-4.20210405git0fd4648.fc34.src.rpm
Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=67573314

My comments:

> 1. I think the License should be "(MIT and LGPLv3 and BSD)"
After thoughtful reading of "Mixed Source Licensing Scenario" and its sample I decided to use 'MIT and (BSD and LGPLv3)' as main sources are MIT-licensed.

> 2. all the licenses and copyright notices included in %license
Done

> 3. qvge.appdata.xml must also be updated to list the actual license that has now been determined
Done

> 4. Add "BuildRequires: make"
Done

> 5. add "Requires: shared-mime-info"
Done

> 6. There is no man page, you should get in contact with the upstream about adding such
I cannot imagine what to write into manpage.
"man qvge: 'to run qvge just run qvge'"?
It has no any CLI option.
> Agreed, although I will note that this is a SHOULD rather than a MUST, and that help2man can be useful sometimes.
and -h not works too.

> 7. %check is recommended, perhaps add a comment line there explaining that upstream does not provide any?
Noted in %check section. It not provides 'make check'

Nice, thank you! Still come comments:

1. Something about the licenses is still bothering me. I'll get back when I have a bit more time to write a full response about it.

2. For AppStream, there should be maximum 1 project_license and multiple licenses encoded using AND notation there. See spec:

https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html#tag-project_license

Strangely, appstream-util validation did not complain, so filed an issue about that:

https://github.com/hughsie/appstream-glib/issues/401  

3. For man page and help, they would be useful to explain the difference between running 'qvge' and 'qvge FILENAME'. But that is a minor thing and adding missing man page is a SHOULD item, so I am completely fine with not having a man page in this case.

4. One point that I only noticed now: appdata.xml should be installed to directory given by macro %{_metainfodir}.  Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/, https://docs.fedoraproject.org/en-US/packaging-guidelines/#_macros

The licensing issues I still have:

1. It really should be "MIT and LGPLv3 and BSD", without splitting it with parenthesis. The crucial question is: What license(s) apply to binary qvgeapp? The answer is, all of them, so the triplet a unit. But since the License field is a rabbit hole when bundled dependecies are present (e.g. we could start discussing the auxiliary files apart from the binary…), so I will just suggest you go with my suggestion, but not demand any changes at this point, close enough I say.

2. qtpropertybrowser's license file is still missing. So either add LICENSE.qtpropertybrowser as well (need to work with upstream here, because the do not have anything suitable, README.qsint is close, but is ruined by its header above the license proper), or change the naming scheme to LICENCE.LGPLv3, etc.

(In reply to Otto Urpelainen from comment #19)
> The licensing issues I still have:
> 
> 1. It really should be "MIT and LGPLv3 and BSD", without splitting it with
> parenthesis. The crucial question is: What license(s) apply to binary
> qvgeapp? The answer is, all of them, so the triplet a unit. But since the
> License field is a rabbit hole when bundled dependecies are present (e.g. we
> could start discussing the auxiliary files apart from the binary…), so I
> will just suggest you go with my suggestion, but not demand any changes at
> this point, close enough I say.

So, we get back to previous, right?
https://tieugene.fedorapeople.org/rpms/qvge/qvge-0.6.2-3.spec

> 2. qtpropertybrowser's license file is still missing. So either add
> LICENSE.qtpropertybrowser as well (need to work with upstream here, because
> the do not have anything suitable, README.qsint is close, but is ruined by
> its header above the license proper), or change the naming scheme to
> LICENCE.LGPLv3, etc.

As far as I know I cannot add license text to other sources with myself (excluding _extra_ordinary situation).
Real use-case: https://bugzilla.redhat.com/show_bug.cgi?id=1845322#
But from other side as a) qtpropertybrowser really has no license text in separate file and b) not maintained for a long time
I'm planning to include head of it's source as license text:
https://github.com/qtproject/qt-solutions/blob/master/qtpropertybrowser/src/qtpropertybrowser.h
As it is not real license text then this will be LICENSE.qtpropertybrowser
PS. README.qsint is because it is not pure license text but is real README file. IMHO I have no right to rename README into LICENSE if upstream decide to name it as is.

(In reply to Eugene A. Pivnev from comment #20)
> (In reply to Otto Urpelainen from comment #19)
> > The licensing issues I still have:
> > 
> > 1. It really should be "MIT and LGPLv3 and BSD", without splitting it with
> > parenthesis. The crucial question is: What license(s) apply to binary
> > qvgeapp? The answer is, all of them, so the triplet a unit. But since the
> > License field is a rabbit hole when bundled dependecies are present (e.g. we
> > could start discussing the auxiliary files apart from the binary…), so I
> > will just suggest you go with my suggestion, but not demand any changes at
> > this point, close enough I say.
> 
> So, we get back to previous, right?
> https://tieugene.fedorapeople.org/rpms/qvge/qvge-0.6.2-3.spec
>

Yes. I still think that the correct solution would be "(MIT and LGPLv3 and BSD)", but Ben's arguments earlier convinced me that the parenthesis are optional in this case.

> > 2. qtpropertybrowser's license file is still missing. So either add
> > LICENSE.qtpropertybrowser as well (need to work with upstream here, because
> > the do not have anything suitable, README.qsint is close, but is ruined by
> > its header above the license proper), or change the naming scheme to
> > LICENCE.LGPLv3, etc.
> 
> As far as I know I cannot add license text to other sources with myself
> (excluding _extra_ordinary situation).
> Real use-case: https://bugzilla.redhat.com/show_bug.cgi?id=1845322#

Best to start with a reference to Licensing Guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text

What you are not allowed to do is to add a license text that cannot be found upstream. The solution is to add it upstream first, then consume the addition in Fedora. Of course in this case, qvge has copied qtpropertybrowser source from somewhere, so they cannot decide on a license, but comply with whatever license is in force.

> But from other side as a) qtpropertybrowser really has no license text in
> separate file and b) not maintained for a long time
> I'm planning to include head of it's source as license text:
> https://github.com/qtproject/qt-solutions/blob/master/qtpropertybrowser/src/
> qtpropertybrowser.h
> As it is not real license text then this will be LICENSE.qtpropertybrowser

That is a good approach. But you should use the head from qvge's copy of that file. It seems Qt has been editing their licenses slightly, since we are packaging what qvge uses, we should use the license that was used to distribute their copy. The difference seems to be tiny in this case, but that is the right license to use.

Do not just patch it into Fedora package, but submit it upstream, then use that for Fedora. If that fails, the guidelines have options for dealing with missing license in case upstream is not willing to fix the issue in their end.

> PS. README.qsint is because it is not pure license text but is real README
> file. IMHO I have no right to rename README into LICENSE if upstream decide
> to name it as is.

That file name is annoying and confusing, but still ok. It is in the licenses directory and the content is clear, so it is good enough.

I noticed that upstream has already merged the fix for licenses path that used multiple project_license entries [1]. On the other front, my pull request to appstream-glib to validate that multiples are not present has been merged [2]. I corrected the situation by submitting a new fix for qvge [3]. It also adds the license for qtpropertybrowser like I suggested. Let us see what upstream thinks of this.

[1]: https://github.com/ArsMasiuk/qvge/commit/017914914a7d8a4c29dae3a4dcbcf9802429a35d
[2]: https://github.com/hughsie/appstream-glib/pull/402
[3]: https://github.com/ArsMasiuk/qvge/pull/152

(In reply to Otto Urpelainen from comment #22)

So, next try:
Spec: https://tieugene.fedorapeople.org/rpms/qvge/qvge.spec
SRPM: https://tieugene.fedorapeople.org/rpms/qvge/qvge-0.6.3-0.1git2a44063.fc34.src.rpm
Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=68274628

Changelog:
- Sources from freshest git commit (with your, Otto, PR)
- License tag contain plain list (again)
- appinfo.xml patch removed due last upstream commit
- added qtprop..browser license from upstream src
- version changed to prerelease
PS. I agreed with developer that he will release 0.6.3 as is right after this package will be approved (as I hope), so sources will not be changed.

Thank you Eugene! The review took quite many rounds, but the package now looks good to me. Review passed. You can now request a dist-git repository for this package as described at https://fedoraproject.org/wiki/Package_Review_Process

(In reply to Otto Urpelainen from comment #24)
> Thank you Eugene! The review took quite many rounds, but the package now
> looks good to me. Review passed. You can now request a dist-git repository
> for this package as described at
> https://fedoraproject.org/wiki/Package_Review_Process

What the factor?

> limb commented 30 minutes ago
> The review is not approved by the assignee of the Bugzilla bug
https://pagure.io/releng/fedora-scm-requests/issue/34123

Oh, probably related to the fact that this bug was unassigned through the whole review. It should have been assigned to me. I will comment to releng ticket.

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/qvge

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

FEDORA-2021-ba65669027 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-ba65669027 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-ba65669027

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

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