[Bug 1031337] Review Request: notion - A tabbed, tiling window manager forked from Ion3

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1031337



--- Comment #7 from Jeff Backus <jeff.backus@xxxxxxxxx> ---
(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.

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





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]