Bug 611010 - Review Request: webkitgtk3 - GTK+ 3 port of webkit
Review Request: webkitgtk3 - GTK+ 3 port of webkit
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Ankur Sinha (FranciscoD)
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-03 01:17 EDT by Matthias Clasen
Modified: 2010-07-18 23:50 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-07-14 15:07:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
sanjay.ankur: fedora‑review+
kevin: fedora‑cvs+


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

  None (edit)
Comment 1 Chen Lei 2010-07-03 02:00:35 EDT
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 02:05:55 EDT
s/conflict with webkitgtk3/conflict with webkitgtk
Comment 3 Chen Lei 2010-07-03 02:15:18 EDT
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 12:58:35 EDT
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 06:37:51 EDT
(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-09 20:10:37 EDT
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-13 21:23:53 EDT
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 05:57:14 EDT
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 06:00:57 EDT
Created attachment 431723 [details]
build log for failed build
Comment 10 Matthias Clasen 2010-07-14 07:42:49 EDT
 
> 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 07:53:34 EDT
(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 11:15:32 EDT
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 11:19:31 EDT
> 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 12:09:16 EDT
(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 12:24:25 EDT
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 12:35:11 EDT
XXX APPROVED XXX
Comment 17 Matthias Clasen 2010-07-14 12:36:53 EDT
New Package CVS Request
=======================
Package Name: webkitgtk3
Short Description: GTK+ 3 port of webkit
Owners: mclasen
Branches:
Comment 18 Kevin Fenzi 2010-07-14 12:49:14 EDT
CVS done (by process-cvs-requests.py).
Comment 19 Matthias Clasen 2010-07-14 15:07:39 EDT
build done.

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