Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1427085 - Review Request: plantumlqeditor - Simple editor for PlantUML
Summary: Review Request: plantumlqeditor - Simple editor for PlantUML
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Raphael Groner
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: qt-reviews
TreeView+ depends on / blocked
 
Reported: 2017-02-27 10:30 UTC by Damian Wrobel
Modified: 2017-07-07 22:55 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-07-07 07:21:16 UTC
projects.rg: fedora-review+


Attachments (Terms of Use)

Description Damian Wrobel 2017-02-27 10:30:51 UTC
Spec URL: https://dwrobel.fedorapeople.org/projects/rpmbuild/SPECS/plantumlqeditor.spec
SRPM URL: https://dwrobel.fedorapeople.org/projects/rpmbuild/SRPMS/plantumlqeditor-1.2-4.git8dc124b.fc24.src.rpm
Description: PlantUML QEditor is a simple editor written in Qt5 for PlantUML.

At a glance:

 - simple PlantUML editor, with a preview,
 - update the diagram while editing,
 - code assistant to insert ready-made code snippets,
 - written in Qt5, so it should run on all platforms supported by Qt5 and PlantUML.

The editor is quite simple: it monitors the editor for changes, and, if any,
runs plantuml to regenerate the image.

The editor also supports an assistant that allows easy insertion of code snippets into the editor. The assistant is defined by a simple XML and a bunch
of icons, one for each snippet.

Fedora Account System Username: dwrobel

Also available at copr: https://copr.fedorainfracloud.org/coprs/dwrobel/plantumlqeditor/

Comment 1 Raphael Groner 2017-03-02 01:01:57 UTC
Are you interested in a review swap? Maybe you can take a look into bug #1428202.

Comment 2 Damian Wrobel 2017-03-06 09:19:22 UTC
(In reply to Raphael Groner from comment #1)
> Are you interested in a review swap? Maybe you can take a look into bug
> #1428202.

Yes, I'm interested. I'll try to look at it either this or next week.

In the meantime upstream merged my few PR's and also Packaging:versioning has changed[1], so please find updated both spec & srpms:

Spec URL: https://dwrobel.fedorapeople.org/projects/rpmbuild/SPECS/plantumlqeditor.spec
SRPM URL: https://dwrobel.fedorapeople.org/projects/rpmbuild/SRPMS/plantumlqeditor-1.2-5.20170306gitedb5f63.fc25.src.rpm

[1] https://fedoraproject.org/wiki/Packaging:Versioning

Comment 3 Raphael Groner 2017-03-09 18:30:16 UTC
Thanks for the review swap. I'll try to gain a deeper insight in the next days.

Comment 4 Raphael Groner 2017-03-29 23:02:34 UTC
Sorry for the very long delay here. I had a lot of daily job work to do.

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

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


Issues:
=======

MUST fix:
- update-desktop-database is invoked in %post and %postun if package
  contains desktop file(s) with a MimeType: entry.
  Note: desktop file(s) with MimeType entry in plantumlqeditor
  See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-
  database
- Can not completely validate GPLv3+ of source files due to missing headers,
  please poke upstream about those:
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/assistantxmlreader.cpp
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/filecache.cpp
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/main.cpp
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/mainwindow.cpp
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/preferencesdialog.cpp
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/previewwidget.cpp
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/recentdocuments.cpp
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/textedit.cpp
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/utils.cpp
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/assistantxmlreader.h
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/filecache.h
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/mainwindow.h
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/preferencesdialog.h
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/previewwidget.h
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/recentdocuments.h
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/settingsconstants.h
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/textedit.h
  plantumlqeditor-edb5f633b3544a35ee9f8fd841f86373bc3dc5ce/utils.h
  (additional tests files can be ignored, they don't get into binary package) 
- Add a license breakdown, see also above about guessable with GPLv3+ files.
  https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios
- Remove duplication of folders ownership, see note below.
  https://fedoraproject.org/wiki/Packaging:Guidelines#The_directory_is_also_owned_by_a_package_implementing_required_functionality_of_your_package
- Note correct release string in changelog, see rpmling warning.
  https://fedoraproject.org/wiki/Package_Versioning_Examples#Complex_versioning_examples
- Unbundle qtsingleapplication, it's available as a separate package.
  See README.qtsingleapplication. 

SHOULD fix:
- May want to use latest commit for the package. As of now: 7a96d90
- Please include README.mime into %doc, it contains useful information
  about mime integration. Even better would be if package can configure that.
- Please move appstream-util validate-relax from %install into %check.
- May be worth to run tests of dedicated subfolder in %check.
- Please use 'cp -p' to preserve timestamps also for icons and translations.
- Please drop useless /usr/lib/.build-id file, see rpmlint warnings.
- Suggestion: Better use clean patches instead of the sed commands to 
  properly adjust configuration.
  https://fedoraproject.org/wiki/Packaging:Guidelines#Fedora-specific_.28or_rejected_upstream.29_patches
- In %build, pushd/popd is preferred over '(cd build-dir; %cmake …)' and more
  common in packages I've seen so far, especially to have locations get logged.
  https://www.eriwen.com/bash/pushd-and-popd/
- May optimize %files and drop all %dir lines and just include complete
  folders by optionally adding a slash ('/') at the end.
  For instance, actually just one line needed instead:
  -%dir %{_datadir}/%{name}
  -%{_datadir}/%{name}/assistant.xml
  -%dir %{_datadir}/%{name}/icons
  -%{_datadir}/%{name}/icons/*
  +%{_datadir}/%{name}/
  http://tldp.org/HOWTO/RPM-HOWTO/build.html#FILES


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

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[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.
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "BSD (3 clause)", "Unknown or generated". 137 files have
     unknown license. Detailed output of licensecheck in /home/builder
     /fedora-review/1427085-plantumlqeditor/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[!]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[!]: Package does not own files or directories owned by other packages.
     Note: Dirs in package are owned also by:
     /usr/share/icons/hicolor/scalable/apps(hicolor-icon-theme, fedora-
     logos), /usr/share/appdata(filesystem),
     /usr/share/applications(filesystem), /usr/share/mime/packages(shared-
     mime-info)
[x]: %build honors applicable compiler flags or justifies otherwise.
[!]: Package contains no bundled libraries without FPC exception.
[!]: Changelog in prescribed format.
[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]: 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]: gtk-update-icon-cache is invoked in %postun and %posttrans if package
     contains icons.
     Note: icons in plantumlqeditor
[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 2 files.
[!]: 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]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[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 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 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:
[x]: update-mime-database is invoked in %post and %postun if package stores
     mime configuration in /usr/share/mime/packages.
     Note: mimeinfo files in: plantumlqeditor
     See:
     http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#mimeinfo
     => False positive, propably due to assembled scriplet.
[?]: 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).
[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in
     plantumlqeditor-debuginfo
     => Ignore. debuginfo is auto-generated.
[?]: Package functions as described.
[!]: 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.
[!]: 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]: Uses parallel make %{?_smp_mflags} macro.
[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: plantumlqeditor-1.2-5.20170306gitedb5f63.fc27.x86_64.rpm
          plantumlqeditor-debuginfo-1.2-5.20170306gitedb5f63.fc27.x86_64.rpm
          plantumlqeditor-1.2-5.20170306gitedb5f63.fc27.src.rpm
plantumlqeditor.x86_64: W: incoherent-version-in-changelog 1.2-5.20170227git95b9e7c ['1.2-5.20170306gitedb5f63.fc27', '1.2-5.20170306gitedb5f63']
plantumlqeditor.x86_64: W: only-non-binary-in-usr-lib
plantumlqeditor.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
plantumlqeditor.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
plantumlqeditor.x86_64: W: no-manual-page-for-binary plantumlqeditor
plantumlqeditor.src: W: spelling-error %description -l en_US plantuml -> plantain
3 packages and 0 specfiles checked; 0 errors, 6 warnings.




Rpmlint (debuginfo)
-------------------
Checking: plantumlqeditor-debuginfo-1.2-5.20170306gitedb5f63.fc27.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
plantumlqeditor.x86_64: W: incoherent-version-in-changelog 1.2-5.20170227git95b9e7c ['1.2-5.20170306gitedb5f63.fc27', '1.2-5.20170306gitedb5f63']
plantumlqeditor.x86_64: W: only-non-binary-in-usr-lib
plantumlqeditor.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
plantumlqeditor.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
plantumlqeditor.x86_64: W: no-manual-page-for-binary plantumlqeditor
2 packages and 0 specfiles checked; 0 errors, 5 warnings.



Requires
--------
plantumlqeditor-debuginfo (rpmlib, GLIBC filtered):

plantumlqeditor (rpmlib, GLIBC filtered):
    /bin/sh
    graphviz
    hicolor-icon-theme
    java
    libQt5Core.so.5()(64bit)
    libQt5Core.so.5(Qt_5)(64bit)
    libQt5Core.so.5(Qt_5.7)(64bit)
    libQt5Gui.so.5()(64bit)
    libQt5Gui.so.5(Qt_5)(64bit)
    libQt5Network.so.5()(64bit)
    libQt5Network.so.5(Qt_5)(64bit)
    libQt5Svg.so.5()(64bit)
    libQt5Svg.so.5(Qt_5)(64bit)
    libQt5Widgets.so.5()(64bit)
    libQt5Widgets.so.5(Qt_5)(64bit)
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libm.so.6()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    plantuml
    rtld(GNU_HASH)
    shared-mime-info



Provides
--------
plantumlqeditor-debuginfo:
    plantumlqeditor-debuginfo
    plantumlqeditor-debuginfo(x86-64)

plantumlqeditor:
    appdata()
    appdata(plantumlqeditor.appdata.xml)
    application()
    application(plantumlqeditor.desktop)
    mimehandler(application/x-plantumlqeditor)
    plantumlqeditor
    plantumlqeditor(x86-64)



Source checksums
----------------
https://github.com/borco/plantumlqeditor/archive/edb5f633b3544a35ee9f8fd841f86373bc3dc5ce.tar.gz#/plantumlqeditor-1.2-20170306gitedb5f63.tar.gz :
  CHECKSUM(SHA256) this package     : 6050c0aae7b2a33c406d30ec7cd49ab1d3ba76dcaf2c60615cfc176af0204182
  CHECKSUM(SHA256) upstream package : 6050c0aae7b2a33c406d30ec7cd49ab1d3ba76dcaf2c60615cfc176af0204182


Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -v -m fedora-rawhide-x86_64 -b 1427085
Buildroot used: fedora-rawhide-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

Comment 5 Raphael Groner 2017-03-29 23:31:51 UTC
Scratch build in rawhide fails currently, see bug #1427085.
For now, successful scratch build tried in f26:
Task info: https://koji.fedoraproject.org/koji/taskinfo?taskID=18670397

Comment 6 Raphael Groner 2017-05-06 06:46:06 UTC
Ping, any news here?

Comment 7 Damian Wrobel 2017-05-10 18:50:36 UTC
(In reply to Raphael Groner from comment #4)
> Sorry for the very long delay here. I had a lot of daily job work to do.
It's absolutely no problem as the same applies to others as well - I'm sorry for the delay.

> 
> Package Review
> ==============
> 
> Legend:
> [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
> [ ] = Manual review needed
> 
> 
> Issues:
> =======
> 
> MUST fix:
> - update-desktop-database is invoked in %post and %postun if package
removed

> - Can not completely validate GPLv3+ of source files due to missing headers,
>   please poke upstream about those:
upstream updated source files


> https://fedoraproject.org/wiki/Packaging:
> LicensingGuidelines#Multiple_Licensing_Scenarios
without bundled qtsingleapplication library it becomes single licensed package

> - Remove duplication of folders ownership, see note below.
removed, except %{_datadir}/icons/hicolor/scalable/apps for which I applied:
https://fedoraproject.org/wiki/Packaging:Guidelines#The_package_you_depend_on_to_provide_a_directory_may_choose_to_own_a_different_directory_in_a_later_version_and_your_package_will_run_unmodified_with_that_later_version

> - Note correct release string in changelog, see rpmling warning.
It's according to https://fedoraproject.org/wiki/Packaging:Versioning#Snapshots.

> - Unbundle qtsingleapplication, it's available as a separate package.
unbundled

> 
> SHOULD fix:
> - May want to use latest commit for the package. As of now: 7a96d90
packaged the latest one (includes updated license header)

> - Please include README.mime into %doc, it contains useful information
>   about mime integration. Even better would be if package can configure that.
it was already configured in the previous version, thus README.mime migh confuse user

> - Please move appstream-util validate-relax from %install into %check.
as per https://fedoraproject.org/wiki/Packaging:AppData#app-data-validate_usage it could be: "(in %check or %install)".


> - May be worth to run tests of dedicated subfolder in %check.
I didn't touch it.

> - Please use 'cp -p' to preserve timestamps also for icons and translations.
I'm using 'cp -a' which is the "same as -dR --preserve=all".

> - Please drop useless /usr/lib/.build-id file, see rpmlint warnings.
Strange, as the package never provided anything in the /usr/lib.


Please find updated spec and SRPM files:
Spec URL: https://dwrobel.fedorapeople.org/projects/rpmbuild/SPECS/plantumlqeditor.spec
SRPM URL: https://dwrobel.fedorapeople.org/projects/rpmbuild/SRPMS/plantumlqeditor-1.2-8.20170403git964d4ef.fc25.src.rpm


(In reply to Raphael Groner from comment #5)
> Scratch build in rawhide fails currently, see bug #1427085.

Koji scratch build (rawhide): https://koji.fedoraproject.org/koji/taskinfo?taskID=19490015
at the time of writing all builds finished successfully except s390x which seems to got stuck in: "Loading logs for task 19490021....".

Comment 8 Raphael Groner 2017-05-14 07:19:57 UTC
> > - Please drop useless /usr/lib/.build-id file, see rpmlint warnings.
> Strange, as the package never provided anything in the /usr/lib.

This is a general bug of rpmlint, see bug #1431408.

I'll continue with the review ASAP.

Comment 9 Raphael Groner 2017-06-04 22:06:24 UTC
Thanks for your fixes, but there is still a need of some important comments …

>> MUST fix:
>> - update-desktop-database is invoked in %post and %postun if package
> removed
It's not fixed, please try again.
http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database

>> - Remove duplication of folders ownership, see note below.
> removed, except %{_datadir}/icons/hicolor/scalable/apps for which I applied:
> https://fedoraproject.org/wiki/Packaging:Guidelines#The_package_you_depend_on_to_provide_a_directory_may_choose_to_own_a_different_directory_in_a_later_version_and_your_package_will_run_unmodified_with_that_later_version
Nearly right. Though, you have to remove ownership of 
%{_datadir}/icons/hicolor and all subdirectories because it's obviousl owned by hicolor-icon-theme and 'Requires: hicolor-icon-theme' is sufficient.

>> https://fedoraproject.org/wiki/Packaging:
>> LicensingGuidelines#Multiple_Licensing_Scenarios
> without bundled qtsingleapplication library it becomes single licensed package>> - Unbundle qtsingleapplication, it's available as a separate package.
> unbundled
Try to unbundle gmock and gtest as well, we've also separate packages of those both. Sorry, I forgot to mention that.
https://fedoraproject.org/wiki/Packaging:Guidelines#Bundling_and_Duplication_of_system_libraries

SHOULD (additionally):
> [?]: Package functions as described.> [?]: %check is present and all tests pass.
You should run the tests provided by upstream in %check.
https://github.com/borco/plantumlqeditor/tree/master/tests

Comment 10 Damian Wrobel 2017-06-06 17:02:40 UTC
(In reply to Raphael Groner from comment #9)
> Thanks for your fixes, but there is still a need of some important comments …
> 
> >> MUST fix:
> >> - update-desktop-database is invoked in %post and %postun if package
> > removed
> It's not fixed, please try again.
> http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database
It's removed and I don't see it in the spec file. The following command proves it:
$ curl -s https://dwrobel.fedorapeople.org/projects/rpmbuild/SPECS/plantumlqeditor.spec | grep update-desktop-database | wc -l
0

> 
> >> - Remove duplication of folders ownership, see note below.
> > removed, except %{_datadir}/icons/hicolor/scalable/apps for which I applied:
> > https://fedoraproject.org/wiki/Packaging:Guidelines#The_package_you_depend_on_to_provide_a_directory_may_choose_to_own_a_different_directory_in_a_later_version_and_your_package_will_run_unmodified_with_that_later_version
> Nearly right. Though, you have to remove ownership of 
> %{_datadir}/icons/hicolor and all subdirectories because it's obviousl owned
> by hicolor-icon-theme and 'Requires: hicolor-icon-theme' is sufficient.

I'll remove it. Although, it not clear to me where it's guaranteed that hicolor-icon-theme will not change directory structure in the future.

> > [?]: %check is present and all tests pass.
> You should run the tests provided by upstream in %check.
> https://github.com/borco/plantumlqeditor/tree/master/tests
As I mentioned, I didn't touch it because upstream does not even provide a method to compile it with a qmake.


Could you please confirm there is no issue with update-desktop-database before I will modify the spec.

Comment 11 Raphael Groner 2017-06-07 05:07:53 UTC
(In reply to Damian Wrobel from comment #10)
> (In reply to Raphael Groner from comment #9)
> > Thanks for your fixes, but there is still a need of some important comments …
> > 
> > >> MUST fix:
> > >> - update-desktop-database is invoked in %post and %postun if package
> > > removed
> > It's not fixed, please try again.
> > http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database
> It's removed and I don't see it in the spec file. The following command
> proves it:
> $ curl -s
> https://dwrobel.fedorapeople.org/projects/rpmbuild/SPECS/plantumlqeditor.
> spec | grep update-desktop-database | wc -l
> 0

In my opinion, we have to ship the calls to update-desktop-database. Is there some misunderstanding of the guidelines?

> > >> - Remove duplication of folders ownership, see note below.
> > > removed, except %{_datadir}/icons/hicolor/scalable/apps for which I applied:
> > > https://fedoraproject.org/wiki/Packaging:Guidelines#The_package_you_depend_on_to_provide_a_directory_may_choose_to_own_a_different_directory_in_a_later_version_and_your_package_will_run_unmodified_with_that_later_version
> > Nearly right. Though, you have to remove ownership of 
> > %{_datadir}/icons/hicolor and all subdirectories because it's obviousl owned
> > by hicolor-icon-theme and 'Requires: hicolor-icon-theme' is sufficient.
> 
> I'll remove it. Although, it not clear to me where it's guaranteed that
> hicolor-icon-theme will not change directory structure in the future.

Let's assume the unobvious option hicolor-icon-theme gets reworked that heavily. Then we'll get a lot of issues with all those packages providing own icons. It's the common way to just R: hicolor-icon-theme .

> > > [?]: %check is present and all tests pass.
> > You should run the tests provided by upstream in %check.
> > https://github.com/borco/plantumlqeditor/tree/master/tests
> As I mentioned, I didn't touch it because upstream does not even provide a
> method to compile it with a qmake.

Please ask upstream how to run the tests.

> Could you please confirm there is no issue with update-desktop-database
> before I will modify the spec.

Confirmed.

Comment 12 Damian Wrobel 2017-06-07 07:49:41 UTC
I've just removed superfluous {_datadir}/icons/hicolor/scalable/apps dir entry.

Please find updated spec and SRPM files:
Spec URL: https://dwrobel.fedorapeople.org/projects/rpmbuild/SPECS/plantumlqeditor.spec
SRPM URL: https://dwrobel.fedorapeople.org/projects/rpmbuild/SRPMS/plantumlqeditor-1.2-9.20170403git964d4ef.fc25.src.rpm

Comment 13 Raphael Groner 2017-06-10 12:46:24 UTC
>> https://fedoraproject.org/wiki/Packaging:
>> LicensingGuidelines#Multiple_Licensing_Scenarios
> without bundled qtsingleapplication library it becomes single licensed package>> - Unbundle qtsingleapplication, it's available as a separate package.
> unbundled
Try to unbundle gmock and gtest as well, we've also separate packages of those both. Sorry, I forgot to mention that.
https://fedoraproject.org/wiki/Packaging:Guidelines#Bundling_and_Duplication_of_system_libraries

- What's the state of this?

Comment 14 Damian Wrobel 2017-06-12 13:52:19 UTC
(In reply to Raphael Groner from comment #13)

> Try to unbundle gmock and gtest as well, we've also separate packages of
> those both. Sorry, I forgot to mention that.
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#Bundling_and_Duplication_of_system_libraries
> 
> - What's the state of this?

I don't have any plans to work on it. My understanding is:
 - upstream even doesn't support building tests with qmake,
 - tests are not "MUST" items,

as a result that shouldn't be a blocking for a review.

Comment 15 Raphael Groner 2017-06-12 15:32:37 UTC
With the assumptions:
- gtest and gmock are bundled only to (optionally) execute the tests and do not get built into distributed binaries.
- The claimed need to call update-desktop-database by scriptlets seems to be a bug in f-r tool because current guidelines mark it as indeed optional.

APPROVED

Thanks for your time and this useful package!

Comment 16 Damian Wrobel 2017-06-13 09:12:23 UTC
Raphael,

Thank you for the review.

Comment 17 Gwyn Ciesla 2017-06-13 15:36:06 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/plantumlqeditor

Comment 18 Fedora Update System 2017-06-13 19:59:14 UTC
plantumlqeditor-1.2-9.20170403git964d4ef.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-2c6e0cf66b

Comment 19 Fedora Update System 2017-06-13 19:59:40 UTC
plantumlqeditor-1.2-9.20170403git964d4ef.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-909ee326ae

Comment 20 Fedora Update System 2017-06-15 10:57:45 UTC
plantumlqeditor-1.2-9.20170403git964d4ef.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-909ee326ae

Comment 21 Fedora Update System 2017-06-15 13:58:23 UTC
plantumlqeditor-1.2-9.20170403git964d4ef.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-2c6e0cf66b

Comment 22 Fedora Update System 2017-07-07 07:21:16 UTC
plantumlqeditor-1.2-9.20170403git964d4ef.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

Comment 23 Fedora Update System 2017-07-07 22:55:07 UTC
plantumlqeditor-1.2-9.20170403git964d4ef.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.