Bug 844165
|
Description
Wolfgang Ulbrich
2012-07-29 13:11:17 UTC
Hi, there is a few thing to fix ( while waiting for mate-conf to be done ) : - %define _default_patch_fuzz 999 this should be dropped. If there is a problem, the patch could be applied incorrectly with issue that would be hard to debug. Not to mention that %define should be replaced by %global - why is there a -devel if it is not mean to be used and linked with ? Either the description is misleading, or there is something using it. - rm -rf $RPM_BUILD_ROOT this should be removed, that's not needed on fedora ( nor on any modern distribution ) - speaking of patches, they should be sent upstream ( ie, to mate, not to gnome ), and if patch are unused, they should be dropped ( or properly rediffed, if they do not apply ). And I think that patching to change defaut is IMHO bad ( like workspaces.patch ), like changing the private API ( Should-set-RestartStyleHint-to-RestartIfRunning-wh.patch ) There is also some patch that were seen as non working by gnome ( http://bugzilla.gnome.org/show_bug.cgi?id=336750 ) - %defattr(-,root,root,-) is uneeded since a long time - %{_bindir}/marco-theme-viewer why is it placed in -devel, as this sound like a useful tool for every day usage ? - %{_mandir}/man1/marco-theme-viewer.1.gz I would recommend to use a joker instead of *.gz, so this would be less work later if the extension is changed. - the reason to run autogen.sh should be documented ( in this case, the patch 19 ), so this would be removed once patch is merged upstream - %{_datadir}/mate/wm-properties/ wouldn't this directory end unowned in the end ? ( and so should be fixed ) - the desktop-file should be validated in the spec : https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage - BuildRequires: libtool automake autoconf gettext BuildRequires: autoconf, automake, libtool, mate-common there is duplicate BuildRequires @ Michael Scherer thx for doing the review :) and for your hints, i will start tomorrow to work on it. @ dependency mate-conf I can use mock because i've a working repo ;) > - %{_bindir}/marco-theme-viewer
> why is it placed in -devel, as this sound like a useful tool for every day
> usage ?
The reason is that the "predecessor" named metacity-theme-viewer also was in the appropriate -devel package. However, I would welcome it in the base package although it is for theme developers, actually not for previews of an installed theme.
Then, that make sense to not put it indeed in the base package, but I am not sure someone wanting to do a theme would find it ( that's not obvious from the description I think ) (In reply to comment #1) > Hi, > > there is a few thing to fix ( while waiting for mate-conf to be done ) : > > > - %define _default_patch_fuzz 999 > > this should be dropped. If there is a problem, the patch could be applied > incorrectly with issue that would be hard to debug. without that line building applying patches failed. At this point i will check if this patches are necessary and can drop safely. [rave@mother ~]$ rpmbuild -ba /home/rave/rpmbuild/SPECS/mate-window-manager.spec Ausführung(%prep): /bin/sh -e /var/tmp/rpm-tmp.5CicTY + umask 022 + cd /home/rave/rpmbuild/BUILD + LANG=C + export LANG + unset DISPLAY + cd /home/rave/rpmbuild/BUILD + rm -rf mate-window-manager-1.4.0 + /usr/bin/xz -dc /home/rave/rpmbuild/SOURCES/mate-window-manager-1.4.0.tar.xz + /bin/tar -xf - + STATUS=0 + '[' 0 -ne 0 ']' + cd mate-window-manager-1.4.0 + /bin/chmod -Rf a+rX,u+w,g-w,o-w . + echo 'Patch #0 (Should-set-RestartStyleHint-to-RestartIfRunning-wh.patch):' Patch #0 (Should-set-RestartStyleHint-to-RestartIfRunning-wh.patch): + /bin/cat /home/rave/rpmbuild/SOURCES/Should-set-RestartStyleHint-to-RestartIfRunning-wh.patch + /usr/bin/patch -s -p1 -b --suffix .Should-set-RestartStyleHint-to-RestartIfRunning-wh --fuzz=0 1 out of 1 hunk FAILED -- saving rejects to file src/core/main.c.rej patch unexpectedly ends in middle of line This happens with patch 0, 19, 20 , 120, 24, 29 I understand your hint, what can i do to get patches work alternative? > Not to mention that %define should be replaced by %global > > > - why is there a -devel if it is not mean to be used and linked with ? > Either the description is misleading, or there is something using it. > > > - rm -rf $RPM_BUILD_ROOT > > this should be removed, that's not needed on fedora ( nor on any modern > distribution ) done > > - speaking of patches, they should be sent upstream ( ie, to mate, not to > gnome ), and if patch are unused, they should be dropped ( or properly > rediffed, if they do not apply ). And I think that patching to change defaut > is IMHO bad ( like workspaces.patch ), like changing the private API ( > Should-set-RestartStyleHint-to-RestartIfRunning-wh.patch ) > > There is also some patch that were seen as non working by gnome ( > http://bugzilla.gnome.org/show_bug.cgi?id=336750 ) I will talk about this in another post, accept workspaces.patch. i remove them because it is nonsense. > - %defattr(-,root,root,-) > is uneeded since a long time thx, for info done > > - %{_bindir}/marco-theme-viewer > why is it placed in -devel, as this sound like a useful tool for every day > usage ? extra post > > - %{_mandir}/man1/marco-theme-viewer.1.gz > I would recommend to use a joker instead of *.gz, so this would be less work > later if the extension is changed. You mean like this? %{_mandir}/man1/marco.*.gz %{_mandir}/man1/marco-message.*.gz done > > - the reason to run autogen.sh should be documented ( in this case, the > patch 19 ), so this would be removed once patch is merged upstream > autogen.sh is needed because all mate upstreams doesn't have configure or make files, only configure.in or .ac and make.in . I will documented it. NOCONFIGURE=1 to avoid double configure I don't understand what you mean with patch 19 > > - %{_datadir}/mate/wm-properties/ > wouldn't this directory end unowned in the end ? ( and so should be fixed ) I will change it to %{_datadir}/mate/wm-properties/marco-wm.desktop > > - the desktop-file should be validated in the spec : > https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file- > install_usage I will read this for understanding > > - BuildRequires: libtool automake autoconf gettext > BuildRequires: autoconf, automake, libtool, mate-common > > there is duplicate BuildRequires You right! I will change it, libtool i remove if current mate-common in stable. I've done a new starting point for this request http://raveit65.fedorapeople.org/Mate-Desktop/fc16/SPECS/mate-window-manager.spec http://raveit65.fedorapeople.org/Mate-Desktop/fc16/SRPM/mate-window-manager-1.4.0-2.fc16.src.rpm oops, i forget change %define to %global but i got it. Created attachment 601359 [details]
Apply-new_windows_always_on_top-to-newly-raised-action
Created attachment 601362 [details]
fedora specific patches
Created attachment 601366 [details] http://forums.mate-desktop.org/viewtopic.php?f=8&t=14 without a them a mysterious window after login with mdm will displayed Created attachment 601367 [details] http://bugzilla.gnome.org/show_bug.cgi?id=558723 without a them a mysterious window after login with mdm will displayed (In reply to comment #18) > Created attachment 601367 [details] > http://bugzilla.gnome.org/show_bug.cgi?id=558723 > > without a them a mysterious window after login with mdm will displayed comment is wrong, sorry new mate-window-manager-1.4.0-3 http://raveit65.fedorapeople.org/Mate-Desktop/fc16/SRPM/mate-window-manager-1.4.0-3.fc16.src.rpm http://raveit65.fedorapeople.org/Mate-Desktop/fc16/SPECS/mate-window-manager.spec %changelog * Mon Jul 30 2012 Wolfgang Ulbrich <chat-to-me> - 1.4.0-3 - remove screenshot-forkbomb.patch - change %%define to %%global * Mon Jul 30 2012 Wolfgang Ulbrich <chat-to-me> - 1.4.0-2 - initial build for fedora - remove workspaces.patch * Sun Dec 25 2011 Wolfgang Ulbrich <chat-to-me> - 1.1.0-1 - mate-window-manager.spec based on metacity-2.34.1-1.fc16 spec No, I mean use foo.1.* , so if the spec is ported on another distribution using lzma, or if Fedora start to use lzma, bz2 or something else, you will have no change to make later. And I do not understand why you have attached all those patches, my point was they go against the spirit of the recommendation for patches : https://fedoraproject.org/wiki/PackageMaintainers/WhyUpstream IE, now there is a new upstream, I think they should really take a look at those patches and see if they are acceptable for them, or say if they reject them. If they do not apply as such, I would take the source code without patch, use patcher ( or git ), and try to apply them one by one, and record the change with git add, and then inspect each patch to see if it look like the old one, if it fixes the issue, if it doesn't introduce a new one ( especially with patch that are applied with the fuzzy option, they can be applied at the wrong location and introduce subtle bugs that upstream developers will never reproduce because they do not have the same patch, giving us a bad reputation ). (In reply to comment #21) > No, I mean use foo.1.* , so if the spec is ported on another distribution > using lzma, or if Fedora start to use lzma, bz2 or something else, you will > have no change to make later. ok, i will change it > > And I do not understand why you have attached all those patches, my point > was they go against the spirit of the recommendation for patches : > https://fedoraproject.org/wiki/PackageMaintainers/WhyUpstream > > IE, now there is a new upstream, I think they should really take a look at > those patches and see if they are acceptable for them, or say if they reject > them. > > If they do not apply as such, I would take the source code without patch, > use patcher ( or git ), and try to apply them one by one, and record the > change with git add, and then inspect each patch to see if it look like the > old one, if it fixes the issue, if it doesn't introduce a new one ( > especially with patch that are applied with the fuzzy option, they can be > applied at the wrong location and introduce subtle bugs that upstream > developers will never reproduce because they do not have the same patch, > giving us a bad reputation ). I will do a test build for me without patches accept patch0, because otherwise i run in this error if someone use mdm (gdm fork) from my repo. http://forums.mate-desktop.org/viewtopic.php?f=8&t=14 patch is send to upstream https://github.com/mate-desktop/mate-window-manager/issues/2 I will ask upstream devs to apply them. Also, i will send the other patches to upstream, let see what they will say. # fedora specific patches Patch12: fresh-tooltips.patch and patches which only applied with fuzz=0 i will droped out in gernerally. @ > - %{_bindir}/marco-theme-viewer > why is it placed in -devel, as this sound like a useful tool for every day > usage ? Can i used it in that form after Mario's comment, or should i update the docomentation? New build comming soon. (In reply to comment #22) > and patches which only applied with fuzz=0 i will droped out in gernerally. > > @ > - %{_bindir}/marco-theme-viewer > > why is it placed in -devel, as this sound like a useful tool for every day > > usage ? i meant fuzz=999 (In reply to comment #21) > IE, now there is a new upstream, I think they should really take a look at > those patches and see if they are acceptable for them, or say if they reject > them. > i found only mate-window-manager-1.4.0 at http://pub.mate-desktop.org/releases/1.4 from 13-Jul-2012 03:25 new koji builds http://raveit65.fedorapeople.org/Mate-Desktop/fc16/SPECS/mate-window-manager.spec http://raveit65.fedorapeople.org/Mate-Desktop/fc16/SRPM/mate-window-manager-1.4.0-4.fc16.src.rpm %changelog * Tue Jul 31 2012 Wolfgang Ulbrich <chat-to-me> - 1.4.0-4 - update to 1.4.1 - remove patch 19, 20, 120, 24, 29 because they only applied with - fuzz=999 - remove other patches for testing acccept patch0 - add joker (.gz to .*) in file section * Mon Jul 30 2012 Wolfgang Ulbrich <chat-to-me> - 1.4.0-3 - remove screenshot-forkbomb.patch - change %%define to %%global * Mon Jul 30 2012 Wolfgang Ulbrich <chat-to-me> - 1.4.0-2 - initial build for fedora - remove workspaces.patch * Sun Dec 25 2011 Wolfgang Ulbrich <chat-to-me> - 1.1.0-1 - mate-window-manager.spec based on metacity-2.34.1-1.fc16 spec I will test this builds on my fc16/fc17 Desktops. Hope that everything is OK without patches. sorry, this is a mock build (In reply to comment #3) > > - %{_bindir}/marco-theme-viewer > > why is it placed in -devel, as this sound like a useful tool for every day > > usage ? > The reason is that the "predecessor" named metacity-theme-viewer also was in > the appropriate -devel package. However, I would welcome it in the base > package although it is for theme developers, actually not for previews of an > installed theme. (In reply to comment #4) > Then, that make sense to not put it indeed in the base package, but I am not > sure someone wanting to do a theme would find it ( that's not obvious from > the description I think ) What about to create a subpackage named -theme-viewer? The -devel package should require that package anyway. Moreover, the docs in %{_datadir}/mate/help/ refer to theme development only, so they should go also in this subpackage. Theme designers don't need header or pc files at all, that's why I mean it would be a good solution. upstream accepted Should-set-RestartStyleHint-to-RestartIfRunning-wh.patch https://github.com/mate-desktop/mate-window-manager/pull/6 Due to your mental instability and immaturity with mate-conf you are no longer welcome as a co-maintainer of the mate-desktop group. Please discontinue any further work on this package. Whatever reason you have to close ( as i assume that I am not the one who is qualified as "instable and immature" since I didn't request co maintainance ), I am still ok to review this package once the needed deps are in. So please do steal assigned bugs from me please. *** This bug has been marked as a duplicate of bug 847725 *** I've redone the package, Wolfgang is more than welcome to review it. *** Bug 847725 has been marked as a duplicate of this bug. *** new starting point SPEC: http://raveit65.fedorapeople.org/Mate-Desktop/f19/SPECS/mate-window-manager.spec SRPM: http://raveit65.fedorapeople.org/Mate-Desktop/f19/SRPM/mate-window-manager-1.4.0-5.fc19.src.rpm Building only works if i downgrade mate-corba in mock to version of my repo. With current version of mate-corba i run into this error. make[3]: Entering directory `/home/rave/rpmbuild/BUILD/mate-window-manager-1.4.0/src' Making all in wm-tester make[4]: Entering directory `/home/rave/rpmbuild/BUILD/mate-window-manager-1.4.0/src/wm-tester' gcc -DHAVE_CONFIG_H -I. -I../.. -pthread -D_REENTRANT -DMATECORBA2=1 -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/mateconf/2 -I/usr/include/matecorba-2.0 -I/usr/include/startup-notification-1.0 -I/usr/include/libgtop-2.0 -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wsign-compare -Wall -ansi -c main.c gcc -DHAVE_CONFIG_H -I. -I../.. -pthread -D_REENTRANT -DMATECORBA2=1 -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/mateconf/2 -I/usr/include/matecorba-2.0 -I/usr/include/startup-notification-1.0 -I/usr/include/libgtop-2.0 -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wsign-compare -Wall -ansi -c test-gravity.c gcc -DHAVE_CONFIG_H -I. -I../.. -pthread -D_REENTRANT -DMATECORBA2=1 -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/mateconf/2 -I/usr/include/matecorba-2.0 -I/usr/include/startup-notification-1.0 -I/usr/include/libgtop-2.0 -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wsign-compare -Wall -ansi -c test-resizing.c gcc -DHAVE_CONFIG_H -I. -I../.. -pthread -D_REENTRANT -DMATECORBA2=1 -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/mateconf/2 -I/usr/include/matecorba-2.0 -I/usr/include/startup-notification-1.0 -I/usr/include/libgtop-2.0 -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wsign-compare -Wall -ansi -c focus-window.c gcc -DHAVE_CONFIG_H -I. -I../.. -pthread -D_REENTRANT -DMATECORBA2=1 -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/mateconf/2 -I/usr/include/matecorba-2.0 -I/usr/include/startup-notification-1.0 -I/usr/include/libgtop-2.0 -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wsign-compare -Wall -ansi -c test-size-hints.c /bin/sh ../../libtool --tag=CC --mode=link gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wsign-compare -Wall -ansi -Wl,-z,relro -o test-gravity test-gravity.o -pthread -lcanberra-gtk -lcanberra -lgtk-x11-2.0 -lgdk-x11-2.0 -latk-1.0 -lgio-2.0 -lpangoft2-1.0 -lpangocairo-1.0 -lgdk_pixbuf-2.0 -lcairo -lpango-1.0 -lfreetype -lfontconfig -lgobject-2.0 -lgmodule-2.0 -lgthread-2.0 -lrt -lmateconf-2 -lstartup-notification-1 -lXcomposite -lXrender -lX11 -lXdamage -lXfixes -lXcursor -lgtop-2.0 -lglib-2.0 -lXext -lXrandr -lXext -lSM -lICE -lX11 -lXext -lXinerama -lm /bin/sh ../../libtool --tag=CC --mode=link gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wsign-compare -Wall -ansi -Wl,-z,relro -o focus-window focus-window.o -pthread -lcanberra-gtk -lcanberra -lgtk-x11-2.0 -lgdk-x11-2.0 -latk-1.0 -lgio-2.0 -lpangoft2-1.0 -lpangocairo-1.0 -lgdk_pixbuf-2.0 -lcairo -lpango-1.0 -lfreetype -lfontconfig -lgobject-2.0 -lgmodule-2.0 -lgthread-2.0 -lrt -lmateconf-2 -lstartup-notification-1 -lXcomposite -lXrender -lX11 -lXdamage -lXfixes -lXcursor -lgtop-2.0 -lglib-2.0 -lXext -lXrandr -lXext -lSM -lICE -lX11 -lXext -lXinerama -lm /bin/sh ../../libtool --tag=CC --mode=link gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wsign-compare -Wall -ansi -Wl,-z,relro -o test-size-hints test-size-hints.o -pthread -lcanberra-gtk -lcanberra -lgtk-x11-2.0 -lgdk-x11-2.0 -latk-1.0 -lgio-2.0 -lpangoft2-1.0 -lpangocairo-1.0 -lgdk_pixbuf-2.0 -lcairo -lpango-1.0 -lfreetype -lfontconfig -lgobject-2.0 -lgmodule-2.0 -lgthread-2.0 -lrt -lmateconf-2 -lstartup-notification-1 -lXcomposite -lXrender -lX11 -lXdamage -lXfixes -lXcursor -lgtop-2.0 -lglib-2.0 -lXext -lXrandr -lXext -lSM -lICE -lX11 -lXext -lXinerama -lm libtool: link: cannot find the library `/usr/lib64/libMateCORBA-2.la' or unhandled argument `/usr/lib64/libMateCORBA-2.la' make[4]: *** [test-gravity] Error 1 make[4]: *** Waiting for unfinished jobs.... libtool: link: cannot find the library `/usr/lib64/libMateCORBA-2.la' or unhandled argument `/usr/lib64/libMateCORBA-2.la' make[4]: *** [focus-window] Error 1 libtool: link: cannot find the library `/usr/lib64/libMateCORBA-2.la' or unhandled argument `/usr/lib64/libMateCORBA-2.la' I need some help for this issue. It builds ok at koji for f18 http://koji.fedoraproject.org/koji/taskinfo?taskID=4398622 (In reply to comment #36) > It builds ok at koji for f18 > > http://koji.fedoraproject.org/koji/taskinfo?taskID=4398622 And rawhide http://koji.fedoraproject.org/koji/taskinfo?taskID=4398656 This means there is a issue with your mock setup. Thanks Leigh, after disable the result repo configuration in mock all is fine. So the review can start now. new starting point. koji scratcch http://koji.fedoraproject.org/koji/taskinfo?taskID=4405613 SPEC: http://raveit65.fedorapeople.org/Mate-Desktop/fc18/SPECS/mate-window-manager.spec SRPM: http://raveit65.fedorapeople.org/Mate-Desktop/fc18/SRPM/mate-window-manager-1.4.0-6.fc18.src.rpm Michael, if you have time we can start now. I don't work for the Mate-Desktop project anymore, because i see no basic for working together with Dan Mashal. |