Bug 1293735 - (boomaga) Review Request: boomaga - A virtual printer for viewing a document before printing
Review Request: boomaga - A virtual printer for viewing a document before pri...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dmitry Mikhirev
Fedora Extras Quality Assurance
http://www.boomaga.org/index.html
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2015-12-22 16:33 EST by MartinKG
Modified: 2016-01-31 16:58 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-01-31 16:58:07 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mikhirev: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description MartinKG 2015-12-22 16:33:25 EST
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/boomaga.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/boomaga-0.7.1-1.fc23.src.rpm

Description:
Boomaga (BOOklet MAnager) is a virtual printer for viewing a document
before printing it out using the physical printer.
The program is very simple to work with.
Running any program, click "print" and select "Boomaga" to see in several
seconds (CUPS takes some time to respond) the Boomaga window open.
If you print out one more document,
it gets added to the previous one, and you can also print them out as one,
and you can also print them out as one.
Regardless of whether your printer supports duplex printing or not,
you would be able to easily print on both sides of the sheet.
If your printer does not support duplex printing,
point this out in the settings, and Booklet would ask you to turn
over the pages half way through printing your document.

The program can also help you get your documents prepared a bit
before printing. At this stage Boomaga makes it possible to:

 * Paste several documents together.
 * Print several pages on one sheet.
 * 1, 2, 4, 8 pages per sheet
 * Booklet. Folding the sheets in two, you'll get a book.

Fedora Account System Username: martinkg

rpmlint boomaga.spec ../SRPMS/boomaga-0.7.1-1.fc23.src.rpm ../RPMS/x86_64/boomaga-*
boomaga.spec: W: invalid-url Source0: boomaga-0.7.1.tar.gz
boomaga.src: W: invalid-url Source0: boomaga-0.7.1.tar.gz
1 packages and 1 specfiles checked; 0 errors, 2 warnings.
Comment 1 MartinKG 2015-12-25 11:34:32 EST
rpm package update:

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/boomaga.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/boomaga-0.7.1-2.git8ca78b2.fc23.src.rpm

rpmlint boomaga.spec ../SRPMS/boomaga-0.7.1-2.git8ca78b2.fc23.src.rpm ../RPMS/x86_64/boomaga-*
3 packages and 1 specfiles checked; 0 errors, 0 warnings.


%changelog
* Fri Dec 25 2015 Martin Gansser <martinkg@fedoraproject.org> - 0.7.1-2.git8ca78b2
- Rebuilt for new git release
Comment 2 Sergio Monteiro Basto 2015-12-25 13:48:57 EST
(In reply to MartinKG from comment #0)
I run fedora-review -b 1293735 -x CheckOwnDirs I got a review for boomaga-0.7.1-1, before you update to 0.7.1-2

[!]: update-mime-database is invoked in %post and %postun if package stores
     mime configuration in /usr/share/mime/packages.
     Note: mimeinfo files in: boomaga
     See:
     http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#mimeinfo


sh: /usr/bin/python: No such file or directory
2 packages and 0 specfiles checked; 0 errors, 0 warnings.

I think you need check python guideline and prepare the package for python3 as default [1] 

I recently have review [2] python-gammu maybe you may follow it , it is very simple all almost done with 2 or 3 macros 

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

[2] https://bugzilla.redhat.com/show_bug.cgi?id=1234654

but I haven't much time
Comment 3 Sergio Monteiro Basto 2015-12-25 14:39:45 EST
(In reply to MartinKG from comment #1)
> - Rebuilt for new git release

Please also use SourceURL guideline [1] like this [2]

[1] https://fedoraproject.org/wiki/Packaging:SourceURL#Git_Tags 

[2] https://pkgs.fedoraproject.org/cgit/rawstudio.git/tree/rawstudio.spec#n19
or 
http://pkgs.fedoraproject.org/cgit/python-gammu.git/tree/python-gammu.spec#n12
Comment 4 Upstream Release Monitoring 2015-12-25 15:16:24 EST
martinkg's scratch build of boomaga-0.7.1-2.git8ca78b2.fc23.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12314612
Comment 5 MartinKG 2015-12-25 17:26:25 EST
(In reply to Sergio Monteiro Basto from comment #2)
> (In reply to MartinKG from comment #0)
> I run fedora-review -b 1293735 -x CheckOwnDirs I got a review for
> boomaga-0.7.1-1, before you update to 0.7.1-2
> 
> [!]: update-mime-database is invoked in %post and %postun if package stores
>      mime configuration in /usr/share/mime/packages.
>      Note: mimeinfo files in: boomaga
>      See:
>      http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#mimeinfo

this part is already in the spec file, it's unclear for me, what i have to do ?

%post
# Install the printer to cups backends
if [ $1 = 1 ]; then
  sh %{_datadir}/%{name}/scripts/installPrinter.sh
fi
/bin/touch --no-create %{_datadir}/icons/hicolor &> /dev/null || :
/bin/touch --no-create %{_datadir}/mime/packages &> /dev/null || :
/usr/bin/update-desktop-database &> /dev/null || :

%postun
/usr/bin/update-desktop-database &> /dev/null || :
if [ $1 -eq 0 ] ; then
    /bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null
    /bin/touch --no-create %{_datadir}/mime/packages &>/dev/null
    /usr/bin/gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || :
    /usr/bin/update-mime-database %{_datadir}/mime &> /dev/null || :
fi

%posttrans
/usr/bin/gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || :
/usr/bin/update-mime-database %{?fedora:-n} %{_datadir}/mime &> /dev/null || :


> 
> sh: /usr/bin/python: No such file or directory
> 2 packages and 0 specfiles checked; 0 errors, 0 warnings.
> 
> I think you need check python guideline and prepare the package for python3
> as default [1] 
> 
> I recently have review [2] python-gammu maybe you may follow it , it is very
> simple all almost done with 2 or 3 macros 
> 
> [1] https://fedoraproject.org/wiki/Packaging:Python
> 
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1234654
> 
> but I haven't much time

I 'am do not understanding, why the package needs python ? and it's totally unclear for me what i have to change in the spec file ?
Comment 6 Dmitry Mikhirev 2015-12-25 18:19:34 EST
An unofficial review.

Need to be fixed:

* Error opening pdf file: cannot find boomagamerger.
* Cups backend and filter directories are %{_prefix}/lib/cups/{backend,filter}, not %{_libdir}/cups/{backend,filter}.
* Directories under %{_datadir}/icons/hicolor must not be owned by this package.
* %post and %preun scripts are inconsistent:

%post
# Install the printer to cups backends
if [ $1 = 1 ]; then
  sh %{_datadir}/%{name}/scripts/installPrinter.sh
fi

%preun
# Uninstall the printer
lpadmin -x "Boomaga"

The printer will be removed on package update. Use 'if [ $1 = 0 ]' in %preun.
Comment 7 Sergio Monteiro Basto 2015-12-25 23:14:38 EST
(In reply to MartinKG from comment #5)
> I 'am do not understanding, why the package needs python ? and it's totally
> unclear for me what i have to change in the spec file ?

Sorry python stuff was a mistake [1] fedora-review shows an error but isn't related with this .spec 

About update-mime-database I pass.

About SourceURL is correct, you just improve what is wiki page , you have a more elegant solution .

my comments are all replied.


[1] https://bugzilla.redhat.com/show_bug.cgi?id=1243292#c10
Comment 8 MartinKG 2015-12-26 06:30:44 EST
(In reply to Dmitry Mikhirev from comment #6)
> An unofficial review.
> 
> Need to be fixed:
> 
> * Error opening pdf file: cannot find boomagamerger.
done

> * Cups backend and filter directories are
> %{_prefix}/lib/cups/{backend,filter}, not %{_libdir}/cups/{backend,filter}.
done

> * Directories under %{_datadir}/icons/hicolor must not be owned by this
> package.
done

> * %post and %preun scripts are inconsistent:
> 
> %post
> # Install the printer to cups backends
> if [ $1 = 1 ]; then
>   sh %{_datadir}/%{name}/scripts/installPrinter.sh
> fi
> 
> %preun
> # Uninstall the printer
> lpadmin -x "Boomaga"
> 
> The printer will be removed on package update. Use 'if [ $1 = 0 ]' in %preun.
done

rpm package update:

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/boomaga.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/boomaga-0.7.1-3.git8ca78b2.fc23.src.rpm

rpmlint boomaga.spec ../SRPMS/boomaga-0.7.1-3.git8ca78b2.fc23.src.rpm ../RPMS/x86_64/boomaga-*
boomaga.spec:59: E: hardcoded-library-path in %{_prefix}/lib/cups/backend
boomaga.spec:60: E: hardcoded-library-path in %{_prefix}/lib/cups/filter
boomaga.spec:119: E: hardcoded-library-path in %{_prefix}/lib/cups
boomaga.spec:120: E: hardcoded-library-path in %{_prefix}/lib/cups/backend
boomaga.spec:122: E: hardcoded-library-path in %{_prefix}/lib/cups/backend/%{name}
boomaga.spec:125: E: hardcoded-library-path in %{_prefix}/lib/cups/filter
boomaga.spec:126: E: hardcoded-library-path in %{_prefix}/lib/cups/filter/boomaga_pstopdf
boomaga.src:59: E: hardcoded-library-path in %{_prefix}/lib/cups/backend
boomaga.src:60: E: hardcoded-library-path in %{_prefix}/lib/cups/filter
boomaga.src:119: E: hardcoded-library-path in %{_prefix}/lib/cups
boomaga.src:120: E: hardcoded-library-path in %{_prefix}/lib/cups/backend
boomaga.src:122: E: hardcoded-library-path in %{_prefix}/lib/cups/backend/%{name}
boomaga.src:125: E: hardcoded-library-path in %{_prefix}/lib/cups/filter
boomaga.src:126: E: hardcoded-library-path in %{_prefix}/lib/cups/filter/boomaga_pstopdf
boomaga.x86_64: W: no-manual-page-for-binary boomagamerger
3 packages and 1 specfiles checked; 14 errors, 1 warnings.


%changelog
* Sat Dec 26 2015 Martin Gansser <martinkg@fedoraproject.org> - 0.7.1-3.git8ca78b2
- Follow https://fedoraproject.org/wiki/Packaging:SourceURL
- corrected cups backend and filter directories
- take ownership of unowned directory %%{_datadir}/icons/hicolor
- use if condition in %%preun script
- linked missing %%{_bindir}/boomagamerger
Comment 9 Dmitry Mikhirev 2016-01-09 09:45:33 EST
OK, now I can finish the review officially.

>> * Error opening pdf file: cannot find boomagamerger.
> done

Well, symlinking to %{_bindir} works, but the proper fix should be patching gui/kernel/tmppdffile.cpp to use compile-time defined path to search boomagamerger instead hardcoded:

 dirs << QApplication::applicationDirPath() + "/../lib/boomaga/";

The correct path can be passed by cmake as macro definition. It is upstream bug, because gui/pdfmerger/CMakeLists.txt respects LIB_SUFFIX, but the code does not. Please open an issue or pull request.

> boomaga.spec:59: E: hardcoded-library-path in %{_prefix}/lib/cups/backend

I'm sorry, that's my mistake. The resulting path is correct now, but another macro should be used: %{_exec_prefix} instead %{_prefix}. The even better option is to use the %{_cups_serverbin} macro provided by the cups-devel package to ensure that the path will remain correct after possible changes in cups packaging.
Comment 10 MartinKG 2016-01-12 16:19:41 EST
(In reply to Dmitry Mikhirev from comment #9)
> OK, now I can finish the review officially.
> 
> >> * Error opening pdf file: cannot find boomagamerger.
> > done
> 
> Well, symlinking to %{_bindir} works, but the proper fix should be patching
> gui/kernel/tmppdffile.cpp to use compile-time defined path to search
> boomagamerger instead hardcoded:
> 
>  dirs << QApplication::applicationDirPath() + "/../lib/boomaga/";
> 
> The correct path can be passed by cmake as macro definition. It is upstream
> bug, because gui/pdfmerger/CMakeLists.txt respects LIB_SUFFIX, but the code
> does not. Please open an issue or pull request.

I reported this bug upstream:
https://github.com/Boomaga/boomaga/issues/32
but the mentioned solution from the developer doesn't work.

> 
> > boomaga.spec:59: E: hardcoded-library-path in %{_prefix}/lib/cups/backend
> 
> I'm sorry, that's my mistake. The resulting path is correct now, but another
> macro should be used: %{_exec_prefix} instead %{_prefix}. The even better
> option is to use the %{_cups_serverbin} macro provided by the cups-devel
> package to ensure that the path will remain correct after possible changes
> in cups packaging.

done
Comment 11 MartinKG 2016-01-20 11:13:00 EST
in which forum can i ask for a solution regarding this issue ?
Comment 12 Dmitry Mikhirev 2016-01-23 14:29:39 EST
Can you show you latest SRPM with fix applied?
Comment 13 MartinKG 2016-01-25 09:56:42 EST
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/boomaga.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/boomaga-0.7.1-4.git2928eef.fc23.src.rpm

changelog
* Sat Jan 09 2016 Martin Gansser <martinkg@fedoraproject.org> - 0.7.1-4.git2928eef
- used %%{_cups_serverbin} macro provided by cups-devel
- Update to new git version
Comment 14 Dmitry Mikhirev 2016-01-25 16:53:06 EST
It works after rebuild on my system. But you posted link to old spec file.
Comment 15 MartinKG 2016-01-25 17:22:39 EST
please can you send me a link to the working boomaga.spec file.
Comment 16 MartinKG 2016-01-27 14:11:25 EST
boomagamerger is an executable file and belongs not into /usr/lib64 but into /usr/bin
Comment 17 Dmitry Mikhirev 2016-01-27 16:21:41 EST
I'm sorry, the rebuild backage contained a symlink in /usr/bin, and that is why it worked. The mistake is that the final slash in path is missing, so the program tries to open /usr/lib64/boomagaboomagamerger file and fails.

> boomagamerger is an executable file and belongs not into /usr/lib64 but into /usr/bin

Not all executable goes to %{_bindir}. It is used for programs that user runs normally, but programs designed to be run by other programs goes to %{_libexecdir} or %{_libdir}/%{name}. See https://fedoraproject.org/wiki/Packaging:Guidelines#Libexecdir for more details.
Comment 18 MartinKG 2016-01-28 03:16:36 EST
Dmitry thanks for your helpfulness and Explanation.

here is the new rpm package:
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/boomaga.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/boomaga-0.7.1-5.git2928eef.fc23.src.rpm

%changelog
* Thu Jan 28 2016 Martin Gansser <martinkg@fedoraproject.org> - 0.7.1-5.git2928eef
- Dropped link for %%{_bindir}/boomagamerger
- Added %%{name}-0.7.1-NONGUI_DIR.patch
Comment 19 Dmitry Mikhirev 2016-01-30 16:55:32 EST
All issues fixed. rpmlint reports no errors/warnings.

Package is APPROVED.
Comment 20 MartinKG 2016-01-30 17:36:22 EST
@Dmitry Thanks for the review.

New Package SCM Request
=======================
Package Name: boomaga 
Short Description: A virtual printer for viewing a document before printing
Owners: martinkg
Branches: f23 rawhide
InitialCC:
Comment 21 Zbigniew Jędrzejewski-Szmek 2016-01-30 18:42:59 EST
Martin, you need to do https://admin.fedoraproject.org/pkgdb/request/package/ instead.
Comment 22 Gwyn Ciesla 2016-01-31 14:31:15 EST
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/boomaga
Comment 23 MartinKG 2016-01-31 16:58:07 EST
package has been built successfully on fc23 and rawhide.

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