Bug 1031337

------                           ------
######                           ######
##############|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.

> For the legal issue, I really don't have idea, as Debian has said it's nonfree.

It's highly likely that they reject the licensing, which is LGPL with sublicensing terms. See beginning of file "LICENSE". It is not permitted to sublicense the LGPL or GPL. If one wants to modify the license, one may derive an own and _renamed_ license from it.

(In reply to Christopher Meng from comment #1)
> 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

Isn't this a window manager?

(In reply to Miro Hrončok from comment #3)
> Isn't this a window manager?

Sorry, it was a mistake.

BTW I posted an email to devel for asking help of choosing location of desktop file for window managers, I think currently the status is very chaotic:

https://lists.fedoraproject.org/pipermail/devel/2013-November/192153.html

Warm Regards.

(In reply to Michael Schwendt from comment #2)
> > For the legal issue, I really don't have idea, as Debian has said it's nonfree.
> 
> It's highly likely that they reject the licensing, which is LGPL with
> sublicensing terms. See beginning of file "LICENSE". It is not permitted to
> sublicense the LGPL or GPL. If one wants to modify the license, one may
> derive an own and _renamed_ license from it.

Yes, this is the reason. At the time, Debian did not accept "tweaked" licenses. I suspect that this is still the case.

(In reply to Christopher Meng from comment #1)
> 
> 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.

Yes, see comment 6. Thanks!

> 
> 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.
> 

Yes, I'd prefer to use fedorapeople.org, but since I am not in a group (yet), I cannot put anything there. Hopefully after I get things spec squared away, though I'm open to suggestions on how to expedite the process :) 

I will make it a point to use direct links into GitHub until then.

> Also note that this is an informal review.

Noted. I am still grateful for any input.

>
> 
> 
> #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.

Unfortunately, the documentation wasn't released by upstream for this version. It is possible that the documentation didn't change since the previous release, in which case I can use that. I'll check it. 


> 
> 3. 
> BuildRequires:  lua
> BuildRequires:  lua-devel
> 
> I think just 
> 
> BuildRequires:  lua-devel
> 
> should be fine.

I may have tried that, I can't remember. My memory is that when I tried it in mock and koji it failed. I believe this is because luac (the Lua compiler) used during build is in lua, not lua-devel.



> 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.

Ah, perhaps that explains why koji failed for the F20 and F21 targets when I was using %doc. Does %{_pkgdocdir} handle subpackages?

> 
> 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.

As I alluded to above, I made an attempt at this and it failed under F20 and F21. I don't believe it was properly picking up the documentation for the subpackages.

> 
> 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.

Sorry, rpmlint suggested %lang(). I'll update.


> 
> 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.
> 

Yes, I'll ping them.

> ***14. This package has bundled libtu and libextl, you are in trouble
> now...***
> 

Yeah, I talked with upstream about this prior to writing the spec. According to the project lead, these "libraries" aren't actually set up to be used outside of Notion. The original developer of Ion intended to fully separate them out into independent libraries but never did. So, in reality they aren't libraries in the traditional sense and therefore I'd argue that the "no bundled libraries" rule doesn't apply.


> ----------------
> 
> 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.

Thanks for taking a look at this! Sorry I broke your brain. :) I'll make the changes and update the spec and SRPM. It may be this weekend before I can address all of the issues and verify that I didn't break anything else.

Updated Spec: http://jsbackus.fedorapeople.org/notion.spec
Updated SRPM: http://jsbackus.fedorapeople.org/notion-3.2013030200-3.fc18.src.rpm

Updated Koji Runs:
* F18: http://koji.fedoraproject.org/koji/taskinfo?taskID=6228340
* F19: http://koji.fedoraproject.org/koji/taskinfo?taskID=6228343
* F20: http://koji.fedoraproject.org/koji/taskinfo?taskID=6228346
* Rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=6228350

Sorry for the delay. Thanks again for reviewing my package, regardless of formality :) 

I've made all requested changes, with the following notes:
* I found alternatives for dropbox:
  - I'm moving the repo for maintaining patches, etc. to fedorahosted.org. I'll keep the GitHub one sync'd for now.
  - Moved transient files (spec and SRPM) to fedorapeople.org.
* I split out the pseudo-libraries libtu/libmainloop/libextl to their own subpackages to better conform to Fedora policy.
* %find_lang didn't pick up the welcome.[fi|cs].txt files, so I marked them with %lang. Hope that's okay.
* No icon and several other WMs don't have one specified, so I'm assuming this is okay.

Assuming I suitably addressed all concerns raised in Christopher's review, my understanding is that the only outstanding issue is Legal's blessing on the license. Is this correct?

New Upstream Release:

Updated Spec: http://jsbackus.fedorapeople.org/notion/notion.spec
Updated SRPM: http://jsbackus.fedorapeople.org/notion/notion-3.2014010900-1.fc18.src.rpm

Koji built packages for F18, F19, and F20 fine. Working to address things flagged by fedora-review. I'll upload an updated spec in the near future.

Received the official response from Fedora legal. The license is considered too restrictive and therefore not suitable for inclusion in Fedora. Moving to RPMFusion and closing:
https://bugzilla.rpmfusion.org/show_bug.cgi?id=3151

Thanks to all for your input!

Regards,
Jeff