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
qt-reviews block added
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
If there is a minimal Qt version required to build, might be a good idea to reflect that in the BuildRequires: dependencies
(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
Taking. I would appreciate it if you would review one of my review requests (e.g. bug 946968 , qt based)
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
(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
%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"
(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
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. :-)
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.
So still -O3 optimization flag is left, please fix. Please try what Kevin suggested, or you may modify cmake-generated files as one method.
Yes, if you set -DCMAKE_BUILD_TYPE=Release, you MUST also set -DCMAKE_CXX_FLAGS_RELEASE:STRING="-O2 -DNDEBUG".
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?
s/expand/explain/g
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.
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.
By the way -O3 issue seems to be already in discussion on bug 875954 .
(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.
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
(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
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
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 --------------------------------------------------
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.)
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.
No SCM request found in bug 929256
Still no SCM request found.
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?
You set the flag to ? once you've included an SCM request: https://fedoraproject.org/wiki/Package_SCM_admin_requests
(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.
New Package SCM Request ======================= Package Name: nomacs Short Description: Qt-based image viewer Owners: tieugene Branches: f17 f18 f19 InitialCC:
> 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).
(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.
Note that I still approve this package.
(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.
Git done (by process-git-requests).
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
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
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
nomacs-1.0.2-3.fc19 has been pushed to the Fedora 19 testing repository.
> 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.
(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.
> 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.
If you just don't specify -DCMAKE_BUILD_TYPE=Release it won't add the -O3.
Seems that "%cmake .." is the best solution: https://bugzilla.redhat.com/show_bug.cgi?id=952632 To Kevin, Mamoru: what you think about this?
Yes, that's what I've been trying to tell you all this time! (You will want to keep the -DENABLE_RAW=1 though.)
nomacs-1.0.2-3.fc17 has been pushed to the Fedora 17 stable repository.
nomacs-1.0.2-3.fc18 has been pushed to the Fedora 18 stable repository.
(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).
nomacs-1.0.2-3.fc19 has been pushed to the Fedora 19 stable repository.