https://bugzilla.redhat.com/show_bug.cgi?id=1031337 Christopher Meng <cickumqt@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |cickumqt@xxxxxxxxx Blocks| |182235 (FE-Legal) --- Comment #1 from Christopher Meng <cickumqt@xxxxxxxxx> --- ------ ------ ###### ###### ##############|Non FREE?|############## ###### ###### ------ ------ This package in Debian is in nonfree repo, have you noticed this fact? I just blocked FE-Legal in order to get some information from RH Legal team. 0. Hope you will get familiar with fedorapeople.org so everytime I don't need to download stuffs from dropbox. Dropbox can't display things directly, so it will make people unsatisfied. Even if you post your spec URL of this: https://github.com/jsbackus/fedora_notion/blob/master/notion.spec will be better than using some download-only services. Also note that this is an informal review. ------ ------ ###### ###### ##############|SPEC part|############## ###### ###### ------ ------ 1. Source0: http://downloads.sourceforge.net/project/notion/notion-3-2013030200-src.tar.bz2 Well, if you don't want to update URL each time you update it, you can: %global majorver 3 %global datever 2013030200 then write your Version tag: Version: %{majorver}.%{datever} And source0: Source0: http://downloads.sourceforge.net/project/notion/%{name}-%{majorver}-%{datever}-src.tar.bz2 And %prep: %setup -q -n %{name}-%{majorver}-%{datever} #Source1: git://notion.git.sourceforge.net/gitroot/notion/notion-doc Source1: https://www.dropbox.com/sh/n1icl72l63dy9tr/jFYmjjqH-f/notion-doc-3-2013030200.tar.bz2 Well, this way is not allowed IMO. # notion.desktop can also be found in git repo https://github.com/jsbackus/fedora_notion.git Source2: https://www.dropbox.com/sh/n1icl72l63dy9tr/Qurc5REVFy/notion.desktop 2. No need to BuildRequires: pkgconfig, RPM can handle this well. 3. BuildRequires: lua BuildRequires: lua-devel I think just BuildRequires: lua-devel should be fine. 4. # This package provides Helvetica 12px. #Requires: xorg-x11-fonts-75dpi Oh, so? Why don't delete these 2 lines? 5. sed -e 's|^\(PREFIX=\).*$|\1/usr|' \ -e 's|^\(ETCDIR=\).*$|\1/etc/notion|' \ -e 's|^\(LUA_DIR=\).*$|\1/usr|' \ -e 's|^\(X11_PREFIX=\).*$|\1/usr|' \ -e 's|^\(X11_LIBS=\).*$|\1`pkg-config --libs x11 xext`|' \ -e 's|^\(LIBDIR=\).*$|\1%{_libdir}|' \ -i system-autodetect.mk Use macro consistently: sed -e 's|^\(PREFIX=\).*$|\1%{_prefix}|' \ -e 's|^\(ETCDIR=\).*$|\1%{_sysconfdir}/%{name}|' \ -e 's|^\(LUA_DIR=\).*$|\1%{_prefix}|' \ -e 's|^\(X11_PREFIX=\).*$|\1%{_prefix}|' \ -e 's|^\(X11_LIBS=\).*$|\1`pkg-config --libs x11 xext`|' \ -e 's|^\(LIBDIR=\).*$|\1%{_libdir}|' \ -i system-autodetect.mk 6. mv $RPM_BUILD_ROOT%{_defaultdocdir}/%{name} $RPM_BUILD_ROOT%{_defaultdocdir}/%{name}-%{version} >From Fedora 20 we change back to %{_defaultdocdir}/%{name} instead of %{_defaultdocdir}/%{name}-%{version} used before f20. Please try %{_pkgdocdir} and see if it helps. Also, these can be handled in system-autodetect.mk, please modify DOCDIR=$(PREFIX)/share/doc/notion to the correct one. 7. # Install and verify desktop file install -Dm0644 %SOURCE2 $RPM_BUILD_ROOT%{_datadir}/xsessions/%{name}.desktop Ah, I don't think this should be put under %{_datadir}/xsessions, you can take a look at what is stored under this location: [rpmaker@fab xsessions]$ ll total 52 -rw-r--r--. 1 root root 268 11月 11 23:40 cinnamon2d.desktop -rw-r--r--. 1 root root 155 11月 11 23:40 cinnamon.desktop -rw-r--r--. 1 root root 1092 11月 13 14:50 enlightenment.desktop -rw-r--r--. 1 root root 5044 10月 16 21:29 gnome-classic.desktop -rw-r--r--. 1 root root 7088 11月 4 08:37 gnome.desktop -rw-r--r--. 1 root root 4785 11月 11 23:13 kde-plasma.desktop -rw-r--r--. 1 root root 7202 11月 11 23:13 kde-plasma-safe.desktop -rw-r--r--. 1 root root 6376 11月 3 01:15 mate.desktop Yeah, all are desktop environments, and user applications should be put under: %{_datadir}/applications BUT, you should install desktop file with `desktop-file-install` command: http://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage 8. mkdir -p $RPM_BUILD_ROOT%{_defaultdocdir}/%{name}-contrib-%{version} for i in LICENSE README; do install -Dm0644 $RPM_BUILD_DIR/%{buildsubdir}/contrib/$i $RPM_BUILD_ROOT%{_defaultdocdir}/%{name}-contrib-%{version}/ done Please adapt to new fedora 20 and, learn how to use %doc macro in %files section instead of doing this. 9. %files section not good: 9.1 %config(noreplace) %{_sysconfdir}/%{name}/* Then you forgot to own the directory %{_sysconfdir}/%{name} itself, please add: %dir %{_sysconfdir}/%{name} 9.2 %{_libdir}/%{name}/bin/* %{_libdir}/%{name}/lc/* %{_libdir}/%{name}/mod/* Well, just %{_libdir}/%{name} will be OK. 9.3 %lang(cs) %{_mandir}/cs/* %lang(fi) %{_mandir}/fi/* %lang(cs) %{_datadir}/locale/cs/* %lang(de) %{_datadir}/locale/de/* %lang(fi) %{_datadir}/locale/fi/* %lang(fr) %{_datadir}/locale/fr/* Please learn how to use %find_lang: http://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files 9.3.1 %lang(fi) %{_datadir}/%{name}/welcome.fi.txt %lang(cs) %{_datadir}/%{name}/welcome.cs.txt Not sure about these 2, but if you use the way mentioned in 9.4, will have duplicated files. 9.4 %{_datadir}/%{name}/ion-completeman %{_datadir}/%{name}/ion-runinxterm %{_datadir}/%{name}/notion-lock %{_datadir}/%{name}/welcome.txt Well, just %{_datadir}/%{name}. 9.5 %{_defaultdocdir}/%{name}-%{version}/README %{_defaultdocdir}/%{name}-%{version}/LICENSE %{_defaultdocdir}/%{name}-%{version}/ChangeLog %{_defaultdocdir}/%{name}-%{version}/RELNOTES As I've said before, please use %doc macro. 9.6 %attr(0644, root, root) %{_datadir}/xsessions/%{name}.desktop When you use desktop-file-utils to handle this, no need to attr() again. 10. %description contrib ..[cut].. Scripts are installed into /usr/share/notion/contrib. To use, copy/link the script(s) you want into ~/.notion and restart Notion. Better using macro to finish: %{_datadir}/%{name}/contrib %package doc Summary: Documentation for the Notion window manager License: GFDL BuildArch: noarch %description doc This package contains the documentation for extending and customizing Notion. %package devel Summary: Development files for the Notion window manager Requires: %{name}%{?_isa} = %{version}-%{release} %description devel This package contains the development files necessary for extending and customizing Notion. Please leave a blank line between package and description. 11. Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.NIgmSp + umask 022 + cd /builddir/build/BUILD + cd notion-3-2013030200 + make -j4 set -e; for i in libmainloop libtu libextl mod_tiling mod_query mod_menu mod_dock mod_sp mod_sm mod_statusbar de mod_xinerama mod_xrandr mod_xkbevents mod_notionflux ioncore notion etc utils man po; do make -C $i; done make[1]: Entering directory `/builddir/build/BUILD/notion-3-2013030200/libmainloop' gcc -Os -W -Wimplicit -Wreturn-type -Wswitch -Wcomment -Wtrigraphs -Wformat -Wchar-subscripts -Wparentheses -pedantic -Wuninitialized -DCF_XFREE86_TEXTPROP_BUG_WORKAROUND -I.. -I.. -I.. -DHAS_SYSTEM_ASPRINTF=1 -g -D_POSIX_C_SOURCE=200112L -c select.c -o select.o You forgot to insert %{optflags} for building: http://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS 12. 4 patches: # Patch submitted to upstream via e-mail on 11/3/2013 Patch0: https://www.dropbox.com/sh/n1icl72l63dy9tr/QlpDOhk8Vc/notion-3.2013030200.p00-man-utf8.patch # Patch submitted to upstream via e-mail on 11/3/2013 Patch1: https://www.dropbox.com/sh/n1icl72l63dy9tr/Pc9uyH5Boo/notion-3.2013030200.p01-fsf_addr.patch # Patch submitted to upstream via e-mail on 11/3/2013 Patch2: https://www.dropbox.com/sh/n1icl72l63dy9tr/dwIWWPddTE/notion-doc-3.2013030200.p02-css_newline.patch # Patch submitted to upstream via e-mail on 11/3/2013 Patch3: https://www.dropbox.com/sh/n1icl72l63dy9tr/_4wS0oLCEX/notion-3.2013030200.p03-ChangeLog_update.patch # Patch submitted to upstream via e-mail on 11/16/2013 Patch4: https://www.dropbox.com/s/ptvk85d3g6h22pn/notion-3.2013030200.p04-fonts.patch Please don't URL with dropbox, it's not OK. You can just write # Patch submitted to upstream via e-mail on 11/3/2013 Patch0: notion-3.2013030200.p00-man-utf8.patch # Patch submitted to upstream via e-mail on 11/3/2013 Patch1: notion-3.2013030200.p01-fsf_addr.patch # Patch submitted to upstream via e-mail on 11/3/2013 Patch2: notion-doc-3.2013030200.p02-css_newline.patch # Patch submitted to upstream via e-mail on 11/3/2013 Patch3: notion-3.2013030200.p03-ChangeLog_update.patch # Patch submitted to upstream via e-mail on 11/16/2013 Patch4: notion-3.2013030200.p04-fonts.patch If you do really want to include URL, please include the URL with official website link, or upstream mailing list link, for patches you don't need to add a full link if it doesn't exist) 13. You have a doc package using checkouted doc by yourself: cd $RPM_BUILD_DIR/%{buildsubdir}/notion-doc make TOPDIR=.. all You can add %{?_smp_mflags} to it, too. Consider get in touch with upstream to release tarball for notion or include them in one tarball. ***14. This package has bundled libtu and libextl, you are in trouble now...*** ------ ------ ###### ###### ##############|Desktop File|############## ###### ###### ------ ------ First quote it to here: [Desktop Entry] Encoding=UTF-8 Name=notion GenericName=Notion Comment=NotIon Window Manager for X Exec=/usr/bin/notion Terminal=false TryExec=/usr/bin/notion Type=Application [Window Manager] SessionManaged=true 15. Please fix Source2 URL, just be Source2: notion.desktop 16. Exec=/usr/bin/notion just be: Exec=notion 17. TryExec=/usr/bin/notion just be: TryExec=notion 18. Do we need to add an icon? 19. Comment=NotIon Window Manager for X Well, except for X, which one else? ;) Just NotIon Window Manager is fine. ---------------- Package has many issues, my brain is broken now. Many hidden issues are not found so far, please fix above and then let us run again. For the legal issue, I really don't have idea, as Debian has said it's nonfree. Referenced Bugs: https://bugzilla.redhat.com/show_bug.cgi?id=182235 [Bug 182235] Fedora Legal Tracker -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review