Bug 1293735 (boomaga) - Review Request: boomaga - A virtual printer for viewing a document before printing
Summary: Review Request: boomaga - A virtual printer for viewing a document before pri...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: boomaga
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Dmitry Mikhirev
QA Contact: Fedora Extras Quality Assurance
URL: http://www.boomaga.org/index.html
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-12-22 21:33 UTC by MartinKG
Modified: 2016-01-31 21:58 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-01-31 21:58:07 UTC
Type: ---
Embargoed:
mikhirev: fedora-review+


Attachments (Terms of Use)

Description MartinKG 2015-12-22 21:33:25 UTC
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 16:34:32 UTC
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> - 0.7.1-2.git8ca78b2
- Rebuilt for new git release

Comment 2 Sergio Basto 2015-12-25 18:48:57 UTC
(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 Basto 2015-12-25 19:39:45 UTC
(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 20:16:24 UTC
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 22:26:25 UTC
(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 23:19:34 UTC
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 Basto 2015-12-26 04:14:38 UTC
(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 11:30:44 UTC
(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> - 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 14:45:33 UTC
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 21:19:41 UTC
(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 16:13:00 UTC
in which forum can i ask for a solution regarding this issue ?

Comment 12 Dmitry Mikhirev 2016-01-23 19:29:39 UTC
Can you show you latest SRPM with fix applied?

Comment 13 MartinKG 2016-01-25 14:56:42 UTC
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> - 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 21:53:06 UTC
It works after rebuild on my system. But you posted link to old spec file.

Comment 15 MartinKG 2016-01-25 22:22:39 UTC
please can you send me a link to the working boomaga.spec file.

Comment 16 MartinKG 2016-01-27 19:11:25 UTC
boomagamerger is an executable file and belongs not into /usr/lib64 but into /usr/bin

Comment 17 Dmitry Mikhirev 2016-01-27 21:21:41 UTC
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 08:16:36 UTC
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> - 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 21:55:32 UTC
All issues fixed. rpmlint reports no errors/warnings.

Package is APPROVED.

Comment 20 MartinKG 2016-01-30 22:36:22 UTC
@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 23:42:59 UTC
Martin, you need to do https://admin.fedoraproject.org/pkgdb/request/package/ instead.

Comment 22 Gwyn Ciesla 2016-01-31 19:31:15 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/boomaga

Comment 23 MartinKG 2016-01-31 21:58:07 UTC
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.