Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=756635 --- Comment #1 from Scott Tsai <scottt.tw@xxxxxxxxx> 2011-12-21 14:59:17 EST --- This turned out to be a somewhat difficult review. Problems: 1. gac can't find config.h. Try: gac tst/testall.g gac will try to run: gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -DSYS_DEFAULT_PATHS="/usr/share/gap" -o /tmp/gac31213/31213_testall.o -I/usr/share/gap -I/usr/bin -DCONFIG_H -c /tmp/gac31213/31213_testall.c Which fails with: In file included from /usr/share/gap/src/compiled.h:8:0, from /tmp/gac31213/31213_testall.c:2: /usr/share/gap/src/system.h:30:20: fatal error: config.h: No such file or directory Since config.h is found at: /usr/share/gap/bin/x86_64-unknown-linux-gnu-gcc/config.h Patching /usr/bin/gac like this works: --- /usr/bin/gac.orig 2011-12-22 02:54:52.249442817 +0800 +++ /usr/bin/gac 2011-12-22 02:55:01.465442409 +0800 @@ -111,9 +111,9 @@ ## c_compile () { echo ${c_compiler} $3 -o $1 -I${gap_dir} \ - -I${gap_bin} -DCONFIG_H -c $2 + -I${gap_binary} -DCONFIG_H -c $2 ${c_compiler} $3 -o $1 -I${gap_dir} \ - -I${gap_bin} -DCONFIG_H -c $2 + -I${gap_binary} -DCONFIG_H -c $2 } 2. You're not shipping sysinfo.gap. This will break compilation of external GAP packages. http://www.gap-system.org/Manuals/doc/htm/ext/CHAP004.htm says: "We suggest that your GAP package contains a file configure which is called with the path of the GAP root directory as parameter. This file then will read sysinfo.gap and set up everything for compiling under the given architecture (for example creating a Makefile from Makefile.in ... The standard GAP distribution contains a GAP package ``example'' whose installation script shows an example way of how to do this." I think this shows that we want something like a "GAP packaging template" that is tested to work with this base GAP package. Time permitting, picking one noarch and one arch specific package from http://www.gap-system.org/Packages/packages.html and try packaging them should produce a workable GAP packaging template ^_^ (I'm half joking. Obviously time and effort is an issue) Debian does have a GAP packaing policy: http://scottt.tw/debian/gap-policy.txt (I extracted it from Debian's gap-doc package for convenience) I don't think we need to follow their naming policy or placing arch specific and noarch files separately in /usr/lib/gap and /usr/share/gap but it's worth reading. Debian's gap-dev has a very different file layout: http://packages.debian.org/sid/amd64/gap-dev/filelist Placing arch specific files in /usr/lib does get rid of one rpmlint warning but in Fedora at least ngspice also places native .o files in /usr/share. I think keeping your exiting file layout is fine if it's possible to build other GAP packages. Debian only has core gap packaged. Frank Lübeck's gapsync binary distribution: http://www.math.rwth-aachen.de:8001/RsyncLinuxGAP/index.html (linked to from the upstream GAP download page so hopefully somewhat sensible) has packages distributed upstream installed. I placed the shell scripts he used to build for x86 and x86_64 here: http://scottt.tw/gapsync/InstForRsync32 http://scottt.tw/gapsync/InstForRsync64 He statically links everything though. (I havn't read the scripts. Just googled around for how people package GAP) 3. The upstream gap download page (http://www.gap-system.org/Download/index.html) suggests running: gap> tst := Filename( DirectoriesLibrary("tst"), "testall.g" ); gap> Read(tst); as a simple self test after installation but gap> DirectoriesLibrary("tst"); fails looking for /usr/share/gap/tst 4. The emacs packaging guidelines suggest you change the Requires and BuildRequires like this: @@ -29,8 +29,8 @@ Patch2: gap-emacs.patch BuildRequires: desktop-file-utils BuildRequires: netpbm-progs -BuildRequires: emacs-nox -BuildRequires: xemacs-devel xemacs-packages-base +BuildRequires: emacs +BuildRequires: xemacs Requires: gzip @@ -65,7 +65,7 @@ Both syntax highlighting and indentation are supported. %package emacs Summary: Edit GAP files with Emacs Group: Applications/Engineering -Requires: %{name} = %{version}-%{release}, emacs(bin) +Requires: %{name} = %{version}-%{release}, emacs(bin) >= %{_emacs_version} BuildArch: noarch %description emacs @@ -86,7 +86,7 @@ GAP Emacs support. Summary: Edit GAP files with XEmacs Group: Applications/Engineering Requires: %{name} = %{version}-%{release} -Requires: xemacs(bin), xemacs-packages-base +Requires: xemacs(bin) >= %{_xemacs_version} BuildArch: noarch See: http://fedoraproject.org/wiki/Packaging:Emacs#Package_Requires I've verified that those BuildRequires do work in mock build 5. For updating the icon cache, the packaging guidelines recommends: %post /bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null || : %postun if [ $1 -eq 0 ] ; then /bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null /usr/bin/gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || : fi %posttrans /usr/bin/gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || : which is marginally more efficient on package updates as gtk-udpate-icon-cache is only run once. See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache 6. debugvim.txt is shipped twice: /usr/share/gap/etc/debugvim.txt (in gap) /usr/share/doc/gap-vim-4.4.12/debugvim.txt (in gap-vim) you should remove the latter since it's related to /usr/share/gap/etc/debug.vim 7. Personally, I recommend adding a comment before "Requires gzip": +# /usr/bin/gap requires gunzip Requires: gzip -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review