Bug 611010 - Review Request: webkitgtk3 - GTK+ 3 port of webkit
Summary: Review Request: webkitgtk3 - GTK+ 3 port of webkit
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Ankur Sinha (FranciscoD)
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-07-03 05:17 UTC by Matthias Clasen
Modified: 2010-07-19 03:50 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-07-14 19:07:39 UTC
Type: ---
Embargoed:
sanjay.ankur: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
build log for failed build (3.56 KB, text/plain)
2010-07-14 10:00 UTC, Ankur Sinha (FranciscoD)
no flags Details

Comment 1 Chen Lei 2010-07-03 06:00:35 UTC
Some comment for webkitgtk3:

1. It looks like locale files conflict with webkitgtk3.

2. webkitgtk3 uses the same tarball as webkitgtk, I think it can be merged into webkitgtk as subpackages like python/python3 modules.
See http://fedoraproject.org/wiki/Packaging/Python#Subpackages

3. The -doc subpackage seems very tiny and only includes a few files, I think I can be merged into webkitgtk3 and -devel subpackage, it'll more suitable to add LICENSE files to the main package.

Comment 2 Chen Lei 2010-07-03 06:05:55 UTC
s/conflict with webkitgtk3/conflict with webkitgtk

Comment 3 Chen Lei 2010-07-03 06:15:18 UTC
If locale files is used by both webkitgtk and webkitgtk3, I think it's reasonable to split out a -common subpackage to from webkitgtk, then move all docs and locale files to this subpackage.

Comment 4 Matthias Clasen 2010-07-06 16:58:35 UTC
The translation conflict is just an oversight, the package is intended to be fully parallel installable.

Doing it in one spec is just messy, you have to build the same tarball twice with different configurations. I would really prefer to keep things the way they are right now.

Comment 5 Chen Lei 2010-07-08 10:37:51 UTC
(In reply to comment #4)
> The translation conflict is just an oversight, the package is intended to be
> fully parallel installable.

Can those translation files be shared between webkitgtk and webkitgtk3? It seems like the translation files have the same md5sum as webkitgtk

Another minor issue should be addressed:
According to the latest Licensing Guidelines, -doc subpackage should be merged to mainpackage.

See http://lists.fedoraproject.org/pipermail/devel/2010-July/138487.html

Comment 6 Matthias Clasen 2010-07-10 00:10:37 UTC
http://mclasen.fedorapeople.org/webkitgtk3.spec
http://mclasen.fedorapeople.org/webkitgtk3-1.3.2-2.fc14.src.rpm    

Here is a revision that fixes the file conflict with webkitgtk and drops the doc subpackage. I don't know if it builds, since koji is down for the weekend, and my system is not up to building webkit locally...

Comment 7 Matthias Clasen 2010-07-14 01:23:53 UTC
Can I please get this reviewed ? 
It is now blocking updates of seed, gnome-games, yelp, devhelp, empathy, epiphany, ...

Comment 8 Ankur Sinha (FranciscoD) 2010-07-14 09:57:14 UTC
REVIEW: 

+ OK
- NA
? ISSUE

--------------------------------------------------------------------------------

+ Package meets naming and packaging guidelines
+ Spec file matches base package name.
+ Spec has consistant macro usage.
+ Meets Packaging Guidelines.
+ License
? License field in spec matches
+ License file included in package
+ Spec in American English
+ Spec is legible.
- Sources match upstream md5sum:
[Ankur@localhost rpmbuild]$ md5sum webkit-1.3.2.tar.gz SOURCES/webkit-1.3.2.tar.gz 
8736b933d059288cdff9f9be64358954  webkit-1.3.2.tar.gz
8736b933d059288cdff9f9be64358954  SOURCES/webkit-1.3.2.tar.gz


- Package needs ExcludeArch
+ BuildRequires correct
+ Spec handles locales/find_lang
- Package is relocatable and has a reason to be.
+ Package has %defattr and permissions on files is good.
? Package has a correct %clean section.
+ Package has correct buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
+ Package is code or permissible content.
 Doc subpackage needed/used.
+ Packages %doc files don't affect runtime.

+ Headers/static libs in -devel subpackage.
+ Spec has needed ldconfig in post and postun
+ .pc files in -devel subpackage/requires pkgconfig
- .so files in -devel subpackage.
+ -devel package Requires: %{name} = %{version}-%{release}
? .la files are removed.

- Package is a GUI app and has a .desktop file

- Package compiles and builds on at least one arch.
+ Package has no duplicate files in %files.
+ Package doesn't own any directories other packages own.
+ Package owns all the directories it creates.
+ No rpmlint output.
Ignorable warnings

- final provides and requires are sane:
(include output of for i in *rpm; do echo $i; rpm -qp --provides $i; echo =; rpm -qp --requires $i; echo; done
manually indented after checking each line.  I also remove the rpmlib junk and anything provided by glibc.)

SHOULD Items:

? Should build in mock.
? Should build on all supported archs
- Should function as described.
- Should have sane scriptlets.
- Should have subpackages require base package with fully versioned depend.
+ Should have dist tag
+ Should package latest version
- check for outstanding bugs on package. (For core merge reviews)

Issues:

1. License would be AML (Apple MIT License) instead of MIT
https://fedoraproject.org/wiki/Licensing
https://fedoraproject.org/wiki/Licensing/Apple_MIT_License

2. clean section missing, assuming this is for F-13 + ?

3. global preferred to define

4. Would the WebCore/WebKit etc be better off as separate packages?

5. fails to build in mock. Build.log attached

6. rpmlint output:
[Ankur@localhost rpmbuild]$ rpmlint SPECS/webkitgtk3.spec SRPMS/webkitgtk3-1.3.2-2.fc14.src.rpm 
SPECS/webkitgtk3.spec:106: W: macro-in-comment %patch1
SPECS/webkitgtk3.spec: W: no-cleaning-of-buildroot %install
SPECS/webkitgtk3.spec: W: no-cleaning-of-buildroot %clean
SPECS/webkitgtk3.spec: W: no-buildroot-tag
SPECS/webkitgtk3.spec: W: no-%clean-section
SPECS/webkitgtk3.spec:4: W: mixed-use-of-spaces-and-tabs (spaces: line 4, tab: line 4)
SPECS/webkitgtk3.spec: W: invalid-url Source0: http://www.webkitgtk.org/webkit-1.3.2.tar.gz HTTP Error 407: Proxy Authentication Required
webkitgtk3.src: W: invalid-url URL: http://www.webkitgtk.org/ HTTP Error 407: Proxy Authentication Required
webkitgtk3.src:106: W: macro-in-comment %patch1
webkitgtk3.src: W: no-cleaning-of-buildroot %install
webkitgtk3.src: W: no-cleaning-of-buildroot %clean
webkitgtk3.src: W: no-buildroot-tag
webkitgtk3.src: W: no-%clean-section
webkitgtk3.src:4: W: mixed-use-of-spaces-and-tabs (spaces: line 4, tab: line 4)
webkitgtk3.src: W: invalid-url Source0: http://www.webkitgtk.org/webkit-1.3.2.tar.gz <urlopen error timed out>
1 packages and 1 specfiles checked; 0 errors, 15 warnings.

7. haven't been able to check for la files yet.

Comment 9 Ankur Sinha (FranciscoD) 2010-07-14 10:00:57 UTC
Created attachment 431723 [details]
build log for failed build

Comment 10 Matthias Clasen 2010-07-14 11:42:49 UTC
 
> Issues:
> 
> 1. License would be AML (Apple MIT License) instead of MIT
> https://fedoraproject.org/wiki/Licensing
> https://fedoraproject.org/wiki/Licensing/Apple_MIT_License

Where have you seen that license ? 
I have looked through all the license files and they are all either LGPL or BSD, as the license field states.

> 2. clean section missing, assuming this is for F-13 + ?

F14+, actually. There is no gtk3 in F13

> 3. global preferred to define

I can fix that, certainly.

> 4. Would the WebCore/WebKit etc be better off as separate packages?

As long as they are shipped in a single tarball, and there is no separate users, I don't see the point.

> 5. fails to build in mock. Build.log attached

Sorry, my last update wasn't build-tested. I'll correct this in the next iteration.

Comment 11 Ankur Sinha (FranciscoD) 2010-07-14 11:53:34 UTC
(In reply to comment #10)
> > Issues:
> > 
> > 1. License would be AML (Apple MIT License) instead of MIT
> > https://fedoraproject.org/wiki/Licensing
> > https://fedoraproject.org/wiki/Licensing/Apple_MIT_License
> 
> Where have you seen that license ? 
> I have looked through all the license files and they are all either LGPL or
> BSD, as the license field states.
> 

webkit-1.3.2/WebCore/LICENSE-APPLE
webkit-1.3.2/WebKit/LICENSE

aren't these AMLs? (I could be wrong)

Comment 12 Matthias Clasen 2010-07-14 15:15:32 UTC
Next try:

http://mclasen.fedorapeople.org/webkitgtk3.spec
http://mclasen.fedorapeople.org/webkitgtk3-1.3.3-1.fc14.src.rpm    

successfully build in koji: 
http://koji.fedoraproject.org/koji/taskinfo?taskID=2318967    

I have cut out some of the conditional stuff, and switched to %global. The parallel installability has been fixed in the new upstream release, so that patch is gone.

I'll double-check those licenses

Comment 13 Matthias Clasen 2010-07-14 15:19:31 UTC
> webkit-1.3.2/WebCore/LICENSE-APPLE
> webkit-1.3.2/WebKit/LICENSE

both of these are BSD. Compare

https://fedoraproject.org/wiki/Licensing/BSD#2ClauseBSD
https://fedoraproject.org/wiki/Licensing/Apple_MIT_License

Comment 14 Ankur Sinha (FranciscoD) 2010-07-14 16:09:16 UTC
(In reply to comment #13)
> > webkit-1.3.2/WebCore/LICENSE-APPLE
> > webkit-1.3.2/WebKit/LICENSE
> 
> both of these are BSD. Compare
> 
> https://fedoraproject.org/wiki/Licensing/BSD#2ClauseBSD
> https://fedoraproject.org/wiki/Licensing/Apple_MIT_License    

hmm.. the body looks more like the BSD license. The only thing I'm confused about is the Copyright statement right at the start :
"Copyright (C) 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved." 

Doesn't that imply it's an Apple License? The BSD License says :
"Copyright 1994-2006 The FreeBSD Project. All rights reserved."

We probably need to get this clarified.

The rest looks good. Approved as soon as the License is clarified :)

Comment 15 Tom "spot" Callaway 2010-07-14 16:24:25 UTC
That license is fine. It is acceptable for the Copyright holder and date to be changed in a license. It is BSD.

Comment 16 Ankur Sinha (FranciscoD) 2010-07-14 16:35:11 UTC
XXX APPROVED XXX

Comment 17 Matthias Clasen 2010-07-14 16:36:53 UTC
New Package CVS Request
=======================
Package Name: webkitgtk3
Short Description: GTK+ 3 port of webkit
Owners: mclasen
Branches:

Comment 18 Kevin Fenzi 2010-07-14 16:49:14 UTC
CVS done (by process-cvs-requests.py).

Comment 19 Matthias Clasen 2010-07-14 19:07:39 UTC
build done.


Note You need to log in before you can comment on or make changes to this bug.