Bug 929256 (nomacs) - Review Request: nomacs - Qt-based image viewer
Summary: Review Request: nomacs - Qt-based image viewer
Keywords:
Status: CLOSED ERRATA
Alias: nomacs
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard: Trivial
Depends On:
Blocks: qt-reviews
TreeView+ depends on / blocked
 
Reported: 2013-03-29 15:36 UTC by Eugene A. Pivnev
Modified: 2013-04-20 19:13 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-04-19 04:48:17 UTC
mtasaka: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Eugene A. Pivnev 2013-03-29 15:36:38 UTC
Spec URL: http://tieugene.fedorapeople.org/rpms/nomacs/nomacs.spec
SRPM URL: http://tieugene.fedorapeople.org/rpms/nomacs/nomacs-1.0.0-1.fc18.src.rpm
Description: nomacs is a free image viewer for windows and Linux systems.
It is licensed under the GNU Public License v3.
nomacs is small, fast and able to handle the most common image formats.
Additionally it is possible to synchronize multiple viewers
running on the same computer or via LAN is possible.
It allows to compare images and spot the differences
e.g. schemes of architects to show the progress).

Koji (f17, f18, f19):
http://koji.fedoraproject.org/koji/taskinfo?taskID=5187434
http://koji.fedoraproject.org/koji/taskinfo?taskID=5187465
http://koji.fedoraproject.org/koji/taskinfo?taskID=5187487

rpmlint:
rpmlint ~/rpmbuild/SPECS/nomacs.spec ~/rpmbuild/SRPMS/nomacs-1.0.0-1.fc18.src.rpm ~/rpmbuild/RPMS/i686/nomacs-1.0.0-1.fc18.i686.rpm 
2 packages and 1 specfiles checked; 0 errors, 0 warnings.

Fedora Account System Username: tieugene

Comment 1 Eugene A. Pivnev 2013-03-29 15:40:00 UTC
qt-reviews block added

Comment 2 Eugene A. Pivnev 2013-03-29 16:53:02 UTC
Release incremented to 2: EL6 disabled (qt too old).
Whiteboard: Trivial added (local fedora-review produces clear template)

Spec URL: http://tieugene.fedorapeople.org/rpms/nomacs/nomacs.spec
SRPM URL: http://tieugene.fedorapeople.org/rpms/nomacs/nomacs-1.0.0-2.fc18.src.rpm

Comment 3 Rex Dieter 2013-03-29 17:11:16 UTC
If there is a minimal Qt version required to build, might be a good idea to reflect that in the BuildRequires: dependencies

Comment 4 Eugene A. Pivnev 2013-03-29 19:05:04 UTC
(In reply to comment #3)
> If there is a minimal Qt version required to build, might be a good idea to
> reflect that in the BuildRequires: dependencies

Fixed.

Release incremented to 3: libraries minimal versions defined.

Spec URL: http://tieugene.fedorapeople.org/rpms/nomacs/nomacs.spec
SRPM URL: http://tieugene.fedorapeople.org/rpms/nomacs/nomacs-1.0.0-3.fc18.src.rpm

Koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5188111
http://koji.fedoraproject.org/koji/taskinfo?taskID=5188126
http://koji.fedoraproject.org/koji/taskinfo?taskID=5188133

Comment 5 Mamoru TASAKA 2013-04-07 07:46:36 UTC
Taking.
I would appreciate it if you would review one of my review requests (e.g. bug 946968 , qt based)

Comment 6 Mamoru TASAKA 2013-04-07 15:05:11 UTC
Some notes:

* description
  - Some part of description is redundant and better to fix
    - "free" image viewer is just redundant. Fedora does not allow
      non-free software.
    - "for windows and Linux systems" is not needed
    - License information is shown in License column, writing
      also in description is just redundant.

* SourceURL:
  - For sourceforge based tarball, please follow:
    https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL#Sourceforge.net

* Compilar flags
  - Optimization level -O2 in %optflags are replaced by -O3,
    which is discouraged on Fedora:
    https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Compiler_flags
----------------------------------------------------------
   227  Building CXX object CMakeFiles/nomacs.dir/src/DkConnection.cpp.o
   228  /usr/bin/c++   -DHAVE_EXIV2_HPP -DLIBRAW_VERSION_14 -DNOMACS_VERSION=\"1.0.0\" -DQT_CORE_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_NO_DEBUG -DQT_NO_DEBUG_OUTPUT -DWITH_OPENCV -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables  -O3 -DNDEBUG -I/usr/include/QtGui -I/usr/include/QtNetwork -I/usr/include/QtCore -I/usr/include/QtDesigner -I/usr/include/QtDeclarative -I/usr/include/QtScriptTools -I/usr/include/QtDBus -I/usr/include/QtXml -I/usr/include/QtSql -I/usr/include/QtOpenGL -I/usr/include/QtMultimedia -I/usr/include/QtXmlPatterns -I/usr/include/QtHelp -I/usr/include/QtUiTools -I/usr/include/QtTest -I/usr/include/QtScript -I/usr/include/QtSvg -I/usr/include/Qt3Support -I/usr/lib/qt4/mkspecs/default -I/usr/include/libraw -I/usr/include/opencv -I/builddir/build/BUILD/nomacs-1.0.0/build -I/builddir/build/BUILD/nomacs-1.0.0/src    -o CMakeFiles/nomacs.dir/src/DkConnection.cpp.o -c /builddir/build/BUILD/nomacs-1.0.0/src/DkConnection.cpp
----------------------------------------------------------
    Please remove -O3 optimization level.

* Updating mime infomation
  - Installed desktop file contains mime information.
    Please follow
    https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database

Comment 7 Eugene A. Pivnev 2013-04-07 16:24:00 UTC
(In reply to comment #6)
> * description
>   - Some part of description is redundant and better to fix
Fixed

> * SourceURL:
>   - For sourceforge based tarball, please follow:
Fixed

> * Compilar flags
>   - Optimization level -O2 in %optflags are replaced by -O3,
>     which is discouraged on Fedora:
>     Please remove -O3 optimization level.

Seems that it is %cmake macro feature/bug.
Nomacs not set -Ox at all.
Try to replace "%cmake" with "cmake" - no one -O3.

> * Updating mime infomation
Fixed.

In addition - version update.

Spec URL: http://tieugene.fedorapeople.org/rpms/nomacs/nomacs.spec
SRPM URL: http://tieugene.fedorapeople.org/rpms/nomacs/nomacs-1.0.2-1.fc17.src.rpm

Comment 8 Kevin Kofler 2013-04-07 17:18:58 UTC
%cmake sets the build type to Release by default, and unfortunately this defaults to -O3 -DNDEBUG in CMake. Most packages override this to vastly different defaults, but nomacs uses the CMake defaults. IMHO, we should really change the default in our CMake packaging from -O3 to -O2.

Anyway, to fix this, add to your %cmake line:
-DCMAKE_CXX_FLAGS_RELEASE:STRING="-O2 -DNDEBUG"

Comment 9 Eugene A. Pivnev 2013-04-07 17:36:43 UTC
(In reply to comment #8)
> %cmake sets the build type to Release by default,

Sorry, but: https://bugzilla.redhat.com/show_bug.cgi?id=919044#c31
:-)

> Anyway, to fix this, add to your %cmake line:
> -DCMAKE_CXX_FLAGS_RELEASE:STRING="-O2 -DNDEBUG"
Fixed.

URLs not changed

Comment 10 Kevin Kofler 2013-04-07 17:48:15 UTC
You're right, it doesn't, and IIRC this very issue is why it doesn't.

But that means the -DCMAKE_BUILD_TYPE=release you're adding is the culprit, not the %cmake macro. :-)

Comment 11 Eugene A. Pivnev 2013-04-08 08:42:48 UTC
I was badly sleeping this night and decided revert cmake flags back (and delete -DCXX...).
Because nomacs not set -O.
Then redundant -O3s are cmake's or %cmake's defaults problem.

URLs not changed.

Comment 12 Mamoru TASAKA 2013-04-08 14:52:14 UTC
So still -O3 optimization flag is left, please fix.
Please try what Kevin suggested, or you may modify cmake-generated
files as one method.

Comment 13 Kevin Kofler 2013-04-08 15:35:42 UTC
Yes, if you set -DCMAKE_BUILD_TYPE=Release, you MUST also set
-DCMAKE_CXX_FLAGS_RELEASE:STRING="-O2 -DNDEBUG".

Comment 14 Eugene A. Pivnev 2013-04-08 18:18:23 UTC
Please - exapand me - _why_.
It seems as dirty hack.
* nomacs not set -O at all
* Fedora requires right -O
* and requires Release for binary rpms
From developer's point of view - Fedora defaults must provides those flags that saticfies Fedora's requirements.
As nomacs not set -O - Fedora defaults must set them as it wants.

It is not so hard for me to set CXX.
But - why?

Comment 15 Eugene A. Pivnev 2013-04-08 20:08:57 UTC
s/expand/explain/g

Comment 16 Kevin Kofler 2013-04-08 23:11:08 UTC
If you want CMake's and/or the %cmake macro's defaults to change, you need to take that up with Orion Poplawski, the primary maintainer of CMake in Fedora.

I am telling you how to make your packages compliant with guidelines and best practices NOW, with CMake being set up the way it is.

Comment 17 Mamoru TASAKA 2013-04-09 00:17:20 UTC
Well, as Kevin says, it is easier to fix build flags locally (i.e. in your srpm) to be compliant with Fedora packaging policy. Of course you can file a bug against cmake, but that means that until cmake side is fixed, this review request won't pass, which is ... _I_ don't want.

Comment 18 Mamoru TASAKA 2013-04-09 00:26:50 UTC
By the way -O3 issue seems to be already in discussion on bug 875954 .

Comment 19 Eugene A. Pivnev 2013-04-09 07:11:40 UTC
(In reply to comment #17)
> Well, as Kevin says, it is easier to fix build flags locally (i.e. in your
> srpm) to be compliant with Fedora packaging policy. Of course you can file a
> bug against cmake, but that means that until cmake side is fixed, this
> review request won't pass, which is ... _I_ don't want.

I think that this is very bad idea (on this way _each_ cmake-based package must be rebuild) - but ok, CXX flags added again.

URLs not changed.

Comment 20 Mamoru TASAKA 2013-04-09 09:15:04 UTC
Please change URLs (especially release number) when you modified spec file and add new %changelog entry appropriately:

https://fedoraproject.org/wiki/Packaging:FrequentlyMadeMistakes?rd=Packaging/FrequentlyMadeMistakes

Comment 21 Eugene A. Pivnev 2013-04-09 09:21:59 UTC
(In reply to comment #20)
> Please change URLs (especially release number) when you modified spec file
> and add new %changelog entry appropriately:
> 
> https://fedoraproject.org/wiki/Packaging:FrequentlyMadeMistakes?rd=Packaging/
> FrequentlyMadeMistakes

Fixed.

Spec URL: http://tieugene.fedorapeople.org/rpms/nomacs/nomacs.spec
SRPM URL: http://tieugene.fedorapeople.org/rpms/nomacs/nomacs-1.0.2-2.fc17.src.rpm

Comment 22 Eugene A. Pivnev 2013-04-09 09:40:01 UTC
CXX flags set to "-O3" only.

Spec URL: http://tieugene.fedorapeople.org/rpms/nomacs/nomacs.spec
SRPM URL: http://tieugene.fedorapeople.org/rpms/nomacs/nomacs-1.0.2-3.fc17.src.rpm

Comment 23 Mamoru TASAKA 2013-04-09 14:24:08 UTC
Well, /usr/bin/update-desktop-database can be %{_bindir}/update-desktop-database, however this is not a blocker.

--------------------------------------------------
   This package (nomacs) is APPROVED by mtasaka
--------------------------------------------------

Comment 24 Kevin Kofler 2013-04-09 16:20:21 UTC
Now what's the point of using
-DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE:STRING="-O2"
? Isn't it exactly as if neither of those flags were passed?

Normally, CMAKE_CXX_FLAGS_RELEASE also contains -DNDEBUG, but you're also dropping that. (And by the way, I don't know whether -DNDEBUG makes a difference for this package or not.)

Comment 25 Eugene A. Pivnev 2013-04-09 19:42:22 UTC
1. "-DCMAKE_BUILD_TYPE=Release" - as you know - %cmake by default not set Release.
2. "-DCMAKE_CXX_FLAGS_RELEASE:STRING="-O2"" - Mr. Tasaka don't want to see -O3 in Makefile. And you recomended to add this string to %cmake arguments to avoid -O3.
That's all.

Comment 26 Gwyn Ciesla 2013-04-09 19:53:40 UTC
No SCM request found in bug 929256

Comment 27 Gwyn Ciesla 2013-04-09 20:10:40 UTC
Still no SCM request found.

Comment 28 Eugene A. Pivnev 2013-04-09 20:19:17 UTC
What?..
1. "The reviewer will review your package. You should fix any blockers that the reviewer identifies. Once the reviewer is happy with the package, the fedora-review flag will be set to +, indicating that the package has passed review. "
Bug is marked with "+"
2. " At this point, you need to make an SCM admin request for your newly approved package. "
2.1. "The fedora-cvs flag is changed by clicking on the edit link next to Flags: in the bug summary. Then you can change its value:

Fedora-cvs-admin-q.png "

I changed fedora-cvs to "?"

What?

Comment 29 Gwyn Ciesla 2013-04-09 20:21:21 UTC
You set the flag to ? once you've included an SCM request:

https://fedoraproject.org/wiki/Package_SCM_admin_requests

Comment 30 Eugene A. Pivnev 2013-04-09 20:32:26 UTC
(In reply to comment #29)
> You set the flag to ? once you've included an SCM request:
> 
> https://fedoraproject.org/wiki/Package_SCM_admin_requests

Ooops... sorry.
They out my mind with these cmake flags :-)
To be continued.

Comment 31 Eugene A. Pivnev 2013-04-09 20:36:42 UTC
New Package SCM Request
=======================
Package Name: nomacs
Short Description: Qt-based image viewer
Owners: tieugene
Branches: f17 f18 f19
InitialCC:

Comment 32 Kevin Kofler 2013-04-09 20:50:40 UTC
> 1. "-DCMAKE_BUILD_TYPE=Release" - as you know - %cmake by default not set
> Release.

But my point is, why do you want to set the build type to Release at all? All it typically does is set some flags, and in this case you don't want either of the 2 flags it sets (we definitely don't want -O3, and you also removed the -DNDEBUG).

Comment 33 Eugene A. Pivnev 2013-04-10 08:11:57 UTC
(In reply to comment #32)
> > 1. "-DCMAKE_BUILD_TYPE=Release" - as you know - %cmake by default not set
> > Release.
> 
> But my point is, why do you want to set the build type to Release at all?
> All it typically does is set some flags, and in this case you don't want
> either of the 2 flags it sets (we definitely don't want -O3, and you also
> removed the -DNDEBUG).

I'm not familiar in cmake, so - propose good flags, thet will satisfy Mamory Tasaka too.

Comment 34 Mamoru TASAKA 2013-04-10 08:18:30 UTC
Note that I still approve this package.

Comment 35 Eugene A. Pivnev 2013-04-10 08:20:54 UTC
(In reply to comment #34)
> Note that I still approve this package.

I can do this in next release.
But we must to solve this problem.
For other cmake-based applications too.

Comment 36 Gwyn Ciesla 2013-04-10 10:42:23 UTC
Git done (by process-git-requests).

Comment 37 Fedora Update System 2013-04-10 13:04:49 UTC
nomacs-1.0.2-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/nomacs-1.0.2-3.fc17

Comment 38 Fedora Update System 2013-04-10 13:10:56 UTC
nomacs-1.0.2-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/nomacs-1.0.2-3.fc18

Comment 39 Fedora Update System 2013-04-10 13:11:59 UTC
nomacs-1.0.2-3.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/nomacs-1.0.2-3.fc19

Comment 40 Fedora Update System 2013-04-10 15:23:50 UTC
nomacs-1.0.2-3.fc19 has been pushed to the Fedora 19 testing repository.

Comment 41 Kevin Kofler 2013-04-10 20:20:51 UTC
> I'm not familiar in cmake, so - propose good flags

My proposal was to simply use:
%cmake -DENABLE_RAW=1 ..
without setting -DCMAKE_BUILD_TYPE=Release. Or is there any particular reason why it's needed?

But the package is acceptable as is, the build flags are fine now.

Comment 42 Eugene A. Pivnev 2013-04-11 04:47:45 UTC
(In reply to comment #41)
> > I'm not familiar in cmake, so - propose good flags
> 
> My proposal was to simply use:
> %cmake -DENABLE_RAW=1 ..
> without setting -DCMAKE_BUILD_TYPE=Release. Or is there any particular
> reason why it's needed?

But %cmake macro has no debug or release flags at all.
And what about -O3?

> But the package is acceptable as is, the build flags are fine now.

This need for future packages - qxkb, qlipper, juffed, qterminal, razorqt etc.

Comment 43 Kevin Kofler 2013-04-11 11:39:06 UTC
> But %cmake macro has no debug or release flags at all.

Most projects don't actually need a build type. While in principle, a CMakeLists.txt can do all sorts of things depending on the build type, the only thing the build type is typically used for on *nix is to add some additional compiler flags (and sometimes linker flags, but by default, the added linker flags are empty). So in most cases (I have not looked closely at this particular project though!), setting CMAKE_BUILD_TYPE=Release just to set CMAKE_CXX_FLAGS_RELEASE to "" or to "-O2" (which is already in the RPM_OPT_FLAGS) does nothing at all.

Also note that the default flags for Release also contain -DNDEBUG and that is usually the main difference between Release and Debug builds, so if you want a true release build, you should set CMAKE_CXX_FLAGS_RELEASE to "-O2 -DNDEBUG" or just "-DNDEBUG", not to just "-O2".


> And what about -O3?

-O3 should not be in the resulting build flags, that's the main thing to ensure. How to do it is up to you.

My point is only that the 2 flags you are adding are probably not necessary. But they are not wrong, just probably not needed.

Comment 44 Orion Poplawski 2013-04-18 15:35:23 UTC
If you just don't specify -DCMAKE_BUILD_TYPE=Release it won't add the -O3.

Comment 45 Eugene A. Pivnev 2013-04-18 19:02:50 UTC
Seems that "%cmake .." is the best solution:
https://bugzilla.redhat.com/show_bug.cgi?id=952632
To Kevin, Mamoru: what you think about this?

Comment 46 Kevin Kofler 2013-04-18 20:26:53 UTC
Yes, that's what I've been trying to tell you all this time! (You will want to keep the -DENABLE_RAW=1 though.)

Comment 47 Fedora Update System 2013-04-19 04:48:20 UTC
nomacs-1.0.2-3.fc17 has been pushed to the Fedora 17 stable repository.

Comment 48 Fedora Update System 2013-04-19 04:53:26 UTC
nomacs-1.0.2-3.fc18 has been pushed to the Fedora 18 stable repository.

Comment 49 Eugene A. Pivnev 2013-04-19 06:01:35 UTC
(In reply to comment #46)
> Yes, that's what I've been trying to tell you all this time! (You will want
> to keep the -DENABLE_RAW=1 though.)

Ok - I will do this in next release (with other bugreport or next nomacs release).

Comment 50 Fedora Update System 2013-04-20 19:13:11 UTC
nomacs-1.0.2-3.fc19 has been pushed to the Fedora 19 stable repository.


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