Spec URL: https://didier.dnsdynamic.net/repository/fc20/geomorph/geomorph.spec SRPM URL: https://didier.dnsdynamic.net/repository/fc20/geomorph/geomorph-0.60.1-1.fc20.src.rpm Description: Geomorph is a height field generator and editor for the Linux operating system. A height field is a kind of topographic map. It is a 2D projection of a 3D landscape. Geomorph generates square images and shows a 3D preview of the resulting landscape. The resulting 2D image can be processed with a tool like Povray for rendering the landscape.
1.) %define major_ver 0.60 You want to use %global here instead. (See http://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define) 2.) You can drop old RPM artifacts These are not necessary anymore: BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) (on top of %install:) %{__rm} -rf %{buildroot} (whole %clean scriptlet) %clean rm -rf $RPM_BUILD_ROOT %defattr(-,root,root,-) 3.) Extra dependencies [lkundrak@odvarok SPECS]$ rpm -qp --requires /home/lkundrak/rpmbuild/RPMS/x86_64/geomorph-0.60.1-1.fc20.x86_64.rpm |grep gtkglext gtkglext-libs libgtkglext-x11-1.0.so.0()(64bit) AutoReq generator creats proper library dependence, you should not require gtkglext-libs explicitely. The same applies to mesa-libGLU. 4.) Consistent use of macros: Use either $RPM_BUILD_ROOT or %{buildroot}. Not both. Also, you're using both "make" and "%{__make}". The double underscore denotes that it's an internal rpm macro -- I suggest you don't needlessly use it. 5.) Locale files dragged in manually: %{_datadir}/locale/*/LC_MESSAGES/geomorph.mo You should use %find_lang instead: http://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files 6.) Please use desktop-file-install to copy the .desktop file It will validate it's correct: http://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage 7.) RPMLint unhappy. Here's (apart from the things already mentioned above) what needs addressing: geomorph.x86_64: E: arch-dependent-file-in-usr-share /usr/share/geomorph/0.60.1/scenes/colmap This package installs an ELF binary in the /usr/share hierarchy, which is reserved for architecture-independent files. It should go to %_bindir or be %excluded. geomorph.x86_64: W: devel-file-in-non-devel-package /usr/share/geomorph/0.60.1/scenes/colmap.c A development file (usually source code) is located in a non-devel package. If you want to include source code in your package, be sure to create a development package. Just drop this. You may want to address the other warnings too.
Also, this won't build in f21 due to format-security bugs: http://kojipkgs.fedoraproject.org//work/tasks/5966/6895966/build.log See this: http://fedoraproject.org/wiki/Changes/FormatSecurity f20 build is successful.
Adding FE_NEEDSPONSOR. I'm willing to sponsor the packager if he wishes.
I've run the thing and received a couple of warnings and errors: [lkundrak@odvarok rpmbuild]$ geomorph /usr/share/geomorph/0.60.1/install-step1-dir: line 9: cd: /usr/local/share/geomorph/0.60/scenes: No such file or directory ls: cannot access *.pov: No such file or directory ls: cannot access *.inc: No such file or directory mkdir: created directory ‘/home/lkundrak/geomorph’ cp: cannot stat ‘/usr/local/share/geomorph/0.60/scenes/*’: No such file or directory mkdir: created directory ‘/home/lkundrak/geomorph/tmp’ which: no povray in (/usr/lib64/qt-3.3/bin:/usr/lib64/ccache:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/home/lkundrak/.local/bin:/home/lkundrak/bin) which: no povray36 in (/usr/lib64/qt-3.3/bin:/usr/lib64/ccache:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/home/lkundrak/.local/bin:/home/lkundrak/bin) which: no povray35 in (/usr/lib64/qt-3.3/bin:/usr/lib64/ccache:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/home/lkundrak/.local/bin:/home/lkundrak/bin) which: no x-povray in (/usr/lib64/qt-3.3/bin:/usr/lib64/ccache:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/home/lkundrak/.local/bin:/home/lkundrak/bin) /usr/share/geomorph/0.60.1/install-step2-rcfile: line 88: type: ./v0_30_new_sections: not found xdg-desktop-menu: file '/usr/local/share/geomorph/geomorph.desktop' does not exist xdg-desktop-icon: file '/usr/local/share/geomorph/geomorph.desktop' does not exist I guess you may want to add the povray dependency and deal with the /usr/local paths (or comment out the thing that tries to install the desktop icon -- it seems useless).
(In reply to Lubomir Rintel from comment #3) > Adding FE_NEEDSPONSOR. > I'm willing to sponsor the packager if he wishes. Yes I wish (Look's like a wedding...)
1. g++ colmap.c -o colmap Use %{_cxx} instead of g++. 2. %{__install} %{SOURCE1} $RPM_BUILD_ROOT%{_datadir}/applications Please read: http://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage
Created attachment 899943 [details] Spec file
(In reply to Christopher Meng from comment #6) > 1. g++ colmap.c -o colmap > > Use %{_cxx} instead of g++. 1. This is a c-program => you are supposed to use a c compiler. 2. The correct rpm-macro corresponding to "g++" would be %{__cxx}, not %{_cxx}. 3. Compilation needs to take into account $RPM_OPT_FLAGS => The spec should use something similar to this: %{__cc} ${RPM_OPT_FLAGS} -o colmap colmap.c
Spec URL: https://didier.dnsdynamic.net/repository/fc20/geomorph/geomorph.spec SRPM URL: https://didier.dnsdynamic.net/repository/fc20/geomorph/geomorph-0.60.1-2.fc20.src.rpm rpmlint report only 2 warnings about missing man page. Upstream don't provide them. Thanks for your help.
Seems reasonably good to me now. I see that you've added the format-security patch; which is great. I'm a bit concerned about amount of stuff done in %prep. I understand it's not very easy to get rid of those, but you still might want to open a ticket upstream (to fix their encodings & FSF address, etc.). (By the way, your server seems to have SSL misconfigured: $ curl https://didier.dnsdynamic.net/repository/fc20/geomorph/geomorph.spec curl: (60) The certificate was signed using a signature algorithm that is disabled because it is not secure. More details here: http://curl.haxx.se/docs/sslcerts.html) Also, there's two tiny issues that you should address before importing the package: 1.) Adding --prefix to %configure is redundant; see 'rpm --eval %configure' 2.) You should take care not to modify timestamps of shared files when converting their encodings: 'mv -f $file.utf8 $file' should be preceded by by 'touch -r $file $file.utf8' I can't see that blocking the review though. Please proceed with http://fedoraproject.org/wiki/Package_SCM_admin_requests APPROVED
What do you think? Does it make sense to mention geomorph on the GIS special interest page, or would you say it's more of an art and 3d design thing? https://fedoraproject.org/wiki/GIS
(In reply to Lubomir Rintel from comment #10) > Seems reasonably good to me now. I see that you've added the format-security > patch; which is great. I am quite concerned about the source code's quality. To me it looks like a ca. 10 years pld student's work, which has never been adjusted to modern Linux demands nor seen actual keep-alive maintainance since then == Low quality and unsafe code. Openly said, I would not have approved this package because of the code quality. > I'm a bit concerned about amount of stuff done in %prep. I understand it's > not very easy to get rid of those, but you still might want to open a ticket > upstream (to fix their encodings & FSF address, etc.). You can simply ignore these FSF address warnings. Addressing these are upstream busyness, but otherwise are of not of any importance to Fedora. The fact rpmlint is complaining about these is an rpmlint sillyness.
(In reply to Ralf Corsepius from comment #12) > (In reply to Lubomir Rintel from comment #10) > > Seems reasonably good to me now. I see that you've added the format-security > > patch; which is great. > > I am quite concerned about the source code's quality. To me it looks like a > ca. 10 years pld student's work, which has never been adjusted to modern > Linux demands nor seen actual keep-alive maintainance since then == Low > quality and unsafe code. Well, it's not exactly a secret that as far as it goes for academic/scientific software, code quality is often not vast and authors tend not to keep up with toolchain developments (I just updated siril today...). That said, this is solemnly packager's responsibility. If he's able to cope with the issues (and it seems to me he is) and the package serves its purpose (would it be submitted for the review if it didn't?) it's good to go. > Openly said, I would not have approved this package because of the code > quality. Thanks for sharing your attitude, but this would be just you making up guidelines. I'm positive that if someone challenged your decision given the relevance of your reasoning it would be overturned.
(In reply to Volker Fröhlich from comment #11) > What do you think? Does it make sense to mention geomorph on the GIS special > interest page, or would you say it's more of an art and 3d design thing? > > https://fedoraproject.org/wiki/GIS In my opinion, it's not a GIS tool, because the primary goal of this app is to create from scratch a pixmap which can be used to generate landscape with 3d tool like povray and co. Landscapes are realistic but not real.
New Package SCM Request ======================= Package Name: geomorph Short Description: A height field editor for Linux Upstream URL: http://geomorph.sourceforge.net Owners: tartare Branches: f19 f20 InitialCC:
Git done (by process-git-requests).
(In reply to Lubomir Rintel from comment #13) > (In reply to Ralf Corsepius from comment #12) > > (In reply to Lubomir Rintel from comment #10) > That said, this is solemnly packager's responsibility. No. One of the basic prerequisites of a package to be packaged for Fedora is "function". It's one of the core purposes of a review to asure this. > If he's able to cope > with the issues (and it seems to me he is) I can't judge - The packager is a new-comer. He may-be, he may-be not. > (would it be submitted for the review if it didn't?) We have seen a lot of low qualtity junk going into Fedora, just because "somebody wanted it", because a reviewer and a submitter were playing review ping-pong, or because reviewers were working carelessly. > > Openly said, I would not have approved this package because of the code > > quality. > > Thanks for sharing your attitude, but this would be just you making up > guidelines. I'm positive that if someone challenged your decision given the > relevance of your reasoning it would be overturned. OK, you leave me no joice but to file bugs against this package.
FYI: All these warnings are serious and likely to be causing malfunctions: utils.c:409:191: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] menus_n_tools.c:44:55: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] menus_n_tools.c:110:85: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] x_alloc.c:256:5: warning: suggest explicit braces to avoid ambiguous 'else' [-Wparentheses] gl_preview.c:1040:4: warning: too many arguments for format [-Wformat-extra-args] render.c:122:4: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long int' [-Wformat=] render.c:122:4: warning: format '%d' expects argument of type 'int', but argument 4 has type 'long int' [-Wformat=] render.c:201:3: warning: pointer targets in passing argument 3 of 'write_png' differ in signedness [-Wpointer-sign] hf_dialog_options.c:47:4: warning: format '%d' expects argument of type 'int', but argument 2 has type 'struct GtkWidget *' [-Wformat=] hf_dialog_options.c:49:3: warning: format '%d' expects argument of type 'int', but argument 2 has type 'struct GtkWidget *' [-Wformat=] main.c:113:6: warning: too many arguments for format [-Wformat-extra-args] stack.c:33:34: warning: assignment from incompatible pointer type thisappinit.c:78:2: warning: initialization from incompatible pointer type thisappinit.c:78:2: warning: (near initialization for 'doc_type_list[0].defaults') thisappinit.c:83:2: warning: initialization from incompatible pointer type thisappinit.c:83:2: warning: (near initialization for 'doc_type_list[0].set_creation_container') thisappinit.c:663:15: warning: array subscript is above array bounds [-Warray-bounds]
(In reply to Ralf Corsepius from comment #17) > (In reply to Lubomir Rintel from comment #13) > > (In reply to Ralf Corsepius from comment #12) > > > (In reply to Lubomir Rintel from comment #10) > > > That said, this is solemnly packager's responsibility. > No. One of the basic prerequisites of a package to be packaged for Fedora is > "function". It's one of the core purposes of a review to asure this. > > > If he's able to cope > > with the issues (and it seems to me he is) > I can't judge - The packager is a new-comer. He may-be, he may-be not. That's fine, noone expects you to judge anyone, especially in a packaging review ticket. Apart from that, the packager was quick to address the format string issue once the need to do so arose. > > (would it be submitted for the review if it didn't?) > We have seen a lot of low qualtity junk going into Fedora, just because > "somebody wanted it", because a reviewer and a submitter were playing review > ping-pong, or because reviewers were working carelessly. Well, in this case the packager was already maintaining a public repository of the packages he's importing. I'm assuming it had a purpose. Luckily I, as well as you, don't have to do arbitrary decisions based upon wild guesses. > > > Openly said, I would not have approved this package because of the code > > > quality. > > > > Thanks for sharing your attitude, but this would be just you making up > > guidelines. I'm positive that if someone challenged your decision given the > > relevance of your reasoning it would be overturned. > OK, you leave me no joice but to file bugs against this package. As I was in a position to give you choices. You're always more than welcome to file tickets about issues you encounter. I strongly suggest that you care about quality of the bug reports and only report and demonstrate actual malfunctions -- not merely reporting potential issues from compiler warnings. (In reply to Ralf Corsepius from comment #18) > FYI: All these warnings are serious and likely to be causing malfunctions: As far as my C knowledge goes, they might be not be serious and might not cause malfunctions. I haven't inspected the code. Note that I'm not advocating writing shitty code or encouraging anyone to ignore the warnings. I'm merely implying you're bringing too much drama into the play. That said, you're very welcome to inspect the code, decide upon actual impact and report bugs. Please don't jump into premature judgements from the presence of warnings themselves! > utils.c:409:191: warning: cast to pointer from integer of different size > [-Wint-to-pointer-cast] > menus_n_tools.c:44:55: warning: cast to pointer from integer of different > size [-Wint-to-pointer-cast] > menus_n_tools.c:110:85: warning: cast to pointer from integer of different > size [-Wint-to-pointer-cast] If nothing uses the full integer range, an actual issue might not be hit. > x_alloc.c:256:5: warning: suggest explicit braces to avoid ambiguous 'else' > [-Wparentheses] Are you serious? This is a style issue, not necessarily an actual bug. > gl_preview.c:1040:4: warning: too many arguments for format > [-Wformat-extra-args] Well, an extra value pushed to stack. If it's the last one, noone is going to notice. > render.c:122:4: warning: format '%d' expects argument of type 'int', but > argument 3 has type 'long int' [-Wformat=] > render.c:122:4: warning: format '%d' expects argument of type 'int', but > argument 4 has type 'long int' [-Wformat=] > render.c:201:3: warning: pointer targets in passing argument 3 of > 'write_png' differ in signedness [-Wpointer-sign] Again, if the full range is not used, noone will encounter issues with these. > hf_dialog_options.c:47:4: warning: format '%d' expects argument of type > 'int', but argument 2 has type 'struct GtkWidget *' [-Wformat=] > hf_dialog_options.c:49:3: warning: format '%d' expects argument of type > 'int', but argument 2 has type 'struct GtkWidget *' [-Wformat=] Well, a bit weird to represent a pointer in decimal; but wouldn't it be weird to communicate an address to the user in the first place? > main.c:113:6: warning: too many arguments for format [-Wformat-extra-args] Again, extra stack value. Weird, but potentially no biggie. > stack.c:33:34: warning: assignment from incompatible pointer type > thisappinit.c:78:2: warning: initialization from incompatible pointer type > thisappinit.c:78:2: warning: (near initialization for > 'doc_type_list[0].defaults') > thisappinit.c:83:2: warning: initialization from incompatible pointer type > thisappinit.c:83:2: warning: (near initialization for > 'doc_type_list[0].set_creation_container') You can cast pointer types back and forth via incompatible one and noone will notice any issue. > thisappinit.c:663:15: warning: array subscript is above array bounds > [-Warray-bounds] This is the only one that it is really hard to excuse. It might be that it's not exploitable and leads to no crash due to dumb luck with alignment, but it would be really nice (and presumably easy) to fix it. Didier, if you are listening, I'm wondering if you could consider addressing at least the last one and communicate these upstream?
(In reply to Ralf Corsepius from comment #17) > (In reply to Lubomir Rintel from comment #13) > > (In reply to Ralf Corsepius from comment #12) > > > (In reply to Lubomir Rintel from comment #10) > > > That said, this is solemnly packager's responsibility. > No. One of the basic prerequisites of a package to be packaged for Fedora is > "function". It's one of the core purposes of a review to asure this. Worry not, Ralf, it functions! I've tried it; see my review input above. If you're able to make it do funny things with carefully crafted input does not change the fact.
(In reply to Lubomir Rintel from comment #19) > Didier, if you are listening, I'm wondering if you could consider addressing > at least the last one and communicate these upstream? Done (just for the last one), a patch was created but I'm working to solve the others warnings. A lot of files must be modified (principally adding missing include files) and the only test I found is to test all features one by one to be sure I'm not breaking something. Communication with upstream is started successfully.
Created attachment 900330 [details] Solve array bounds
You might have noticed your repository has been created. Please go ahead, import and build the package; you can close this ticket then. Feel free to address the issues in the git repository as the fixes are ready.
Successfully built on koji for f19, f20 and rawhide
(In reply to Lubomir Rintel from comment #20) > (In reply to Ralf Corsepius from comment #17) > > (In reply to Lubomir Rintel from comment #13) > > > (In reply to Ralf Corsepius from comment #12) > > > > (In reply to Lubomir Rintel from comment #10) > > > > > That said, this is solemnly packager's responsibility. > > No. One of the basic prerequisites of a package to be packaged for Fedora is > > "function". It's one of the core purposes of a review to asure this. > > Worry not, Ralf, it functions! Well, apparently my understanding of SW-quality is different than yours. To me your review is proof of Fedora reviews not working at all.