Bug 691541 - Review Request: icedtea-web - Additional Java components for OpenJDK
Review Request: icedtea-web - Additional Java components for OpenJDK
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Omair Majid
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-28 15:19 EDT by Deepak Bhole
Modified: 2011-04-27 03:06 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-04-27 03:06:04 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
dbhole: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Deepak Bhole 2011-03-28 15:19:14 EDT
Spec URL: http://people.redhat.com/dbhole/scratch/icedtea-web.spec
SRPM URL: http://people.redhat.com/dbhole/scratch/icedtea-web-1.0.1-1.fc15.src.rpm
Description:

The IcedTea-Web project provides a Java web browser plugin, an implementation
of Java Web Start (originally based on the Netx project) and a settings tool to
manage deployment settings for the aforementioned plugin and Web Start
implementations.
Comment 1 Omair Majid 2011-03-28 16:55:02 EDT
Packaging Guidelines:

- Naming: OK.
- Version and Release: OK
- MD5 sum of source tar matches with upstream: OK
- Spec Legibility: OK
- Architecture Support: Issues
Any reason for ExclusiveArch?
- File System Layout: OK
Ideally icedtea-web should install into a normal prefix, not into a JDK dir.But since this is how upstream is handling it _and_ how this was handled when it was a part of java-1.6.0-openjdk-plugin, this is OK for now.
- rpmlint: OK
icedtea-web.spec:97: W: configure-without-libdir-spec
This is the same problem as above. OK for now.
icedtea-web.spec:42: W: mixed-use-of-spaces-and-tabs (spaces: line 11, tab: line 42)
Please fix this?
- Requires: OK
- BuildRequires: OK
- Summary and Description: OK
- Documentation: Issues
Please use %{_javadocdir} for javadoc directory path. The javadoc package needs to be declared as noarch.
- Compiler Flags: Issues
Compiler flags (RPM_OPT_FLAGS, etc) are not being passed.
- DebugInfo Packages: OK
- Devel Packages: N/A
- Requiring Base Package: Issues
The Javadocs dont depend on the main package for functionality; please remove the requires on main package.
- Static and Shared Libraries: OK
- Desktop files: OK
- Macros: Issues
%define used instead of %global. Please use %global
- Scriptlets: OK
I dont understand why only the first alternatives command in %post is if'd. Shouldnt the second alternatives invocation be if'd as well?
- File and directory ownership: OK
- Others:
Please remove all invocations of rm -rf $RPM_BUILD_ROOT. It is unneeded on recent version of Fedora.
Comment 2 Deepak Bhole 2011-03-28 18:01:06 EDT
(In reply to comment #1)
> Packaging Guidelines:
> 
> - Naming: OK.
> - Version and Release: OK
> - MD5 sum of source tar matches with upstream: OK
> - Spec Legibility: OK
> - Architecture Support: Issues
> Any reason for ExclusiveArch?
> - File System Layout: OK
> Ideally icedtea-web should install into a normal prefix, not into a JDK dir.But
> since this is how upstream is handling it _and_ how this was handled when it
> was a part of java-1.6.0-openjdk-plugin, this is OK for now.
> - rpmlint: OK
> icedtea-web.spec:97: W: configure-without-libdir-spec
> This is the same problem as above. OK for now.
> icedtea-web.spec:42: W: mixed-use-of-spaces-and-tabs (spaces: line 11, tab:
> line 42)
> Please fix this?

Fixed.

> - Requires: OK
> - BuildRequires: OK
> - Summary and Description: OK
> - Documentation: Issues
> Please use %{_javadocdir} for javadoc directory path. The javadoc package needs

Fixed.

> to be declared as noarch.
> - Compiler Flags: Issues
> Compiler flags (RPM_OPT_FLAGS, etc) are not being passed.

Fixed.

> - DebugInfo Packages: OK
> - Devel Packages: N/A
> - Requiring Base Package: Issues
> The Javadocs dont depend on the main package for functionality; please remove
> the requires on main package.

Done.

> - Static and Shared Libraries: OK
> - Desktop files: OK
> - Macros: Issues
> %define used instead of %global. Please use %global
> - Scriptlets: OK
> I dont understand why only the first alternatives command in %post is if'd.
> Shouldnt the second alternatives invocation be if'd as well?

Actually, the if in %post shouldn't be an if at all. I had copied and pasted it from the original rpm. We want it to run each time. The if has been removed now.

The one in %postun needs to stay in an if as we want it to run only during uninstall and not during upgrade.

> - File and directory ownership: OK
> - Others:
> Please remove all invocations of rm -rf $RPM_BUILD_ROOT. It is unneeded on
> recent version of Fedora.

Done.

New spec file and srpm uploaded.
Comment 3 Omair Majid 2011-03-29 10:41:13 EDT
Thanks for the fixes. Some more minor issues.

- archbuild, archinstall: are these macros used anywhere? Or is this some rpm-internal magic?

- Why is the main package exclusive arch? Is it because openjdk is exclusive arch?

- Javadoc subpackage needs to be built as noarch [1]. Since it does not depend on the main package, it needs its own copy of COPYING [2].

- %files javadoc uses %{datadir}/javadoc but configure uses %{_javadocdir}

[1] http://fedoraproject.org/wiki/Packaging/Java#Javadoc_installation
[2] http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#Subpackage_Licensing
Comment 4 Alexander Kurtakov 2011-03-30 02:36:41 EDT
Omair, you should set fedora-review flag to ? when starting review and to + when package is approved. I've changed the bug status to assigned which also must be done by the reviewer.
Comment 5 Omair Majid 2011-03-30 10:36:20 EDT
(In reply to comment #4)
> Omair, you should set fedora-review flag to ? when starting review and to +
> when package is approved. I've changed the bug status to assigned which also
> must be done by the reviewer.

Sorry, I wasnt aware of that. Done now.
Comment 6 Deepak Bhole 2011-03-31 15:53:40 EDT
(In reply to comment #3)
> Thanks for the fixes. Some more minor issues.
> 
> - archbuild, archinstall: are these macros used anywhere? Or is this some
> rpm-internal magic?
> 

archbuild was used in an older version of the spec file, not any more though. I've removed it. archinstall is used in the %install section of the spec file.

> - Why is the main package exclusive arch? Is it because openjdk is exclusive
> arch?
> 

Yes. I've added a comment stating so now.

> - Javadoc subpackage needs to be built as noarch [1]. Since it does not depend
> on the main package, it needs its own copy of COPYING [2].
> 

Fixed.

> - %files javadoc uses %{datadir}/javadoc but configure uses %{_javadocdir}
> 

Fixed.

New spec file and srpm attached.
Comment 8 Omair Majid 2011-03-31 16:26:20 EDT
(In reply to comment #7)
> Sorry, new spec file and srpm uploaded I meant:
> 
> http://people.redhat.com/dbhole/scratch/icedtea-web.spec
> http://people.redhat.com/dbhole/scratch/icedtea-web-1.0.1-1.fc15.src.rpm

You have added a patch, but there is no bug number or note that says it's actually an upstream patch backported to fix a bug. It would be great if you could add that.
Comment 9 Deepak Bhole 2011-03-31 16:33:38 EDT
Oops, sorry. That patch was intended to go in (not yet). I was just testing something else. 

I've removed it and re-uploaded.
Comment 10 Omair Majid 2011-03-31 17:19:13 EDT
Looks good to me now.

Approved.
Comment 11 Deepak Bhole 2011-03-31 17:20:21 EDT
Setting review to +
Comment 12 Jason Tibbitts 2011-03-31 19:05:08 EDT
There is no SCM request to process here.
Please provide one and set the fedora-cvs flag back to '?'.
https://fedoraproject.org/wiki/Package_SCM_admin_requests
Comment 13 Deepak Bhole 2011-03-31 19:09:09 EDT
Sorry about that. Info:

New Package SCM Request
=======================
Package Name: icedtea-web
Short Description: Additional Java components for OpenJDK
Owners: dbhole@redhat.com
Branches: f15
InitialCC: dbhole@redhat.com
Comment 14 Jason Tibbitts 2011-04-03 23:56:39 EDT
Please note that SCM requests should reference FAS account names, not email
addresses.  I've fixed things up.

Git done (by process-git-requests).
Comment 15 Alexander Kurtakov 2011-04-27 03:06:04 EDT
Built in koji weeks ago.
http://koji.fedoraproject.org/koji/buildinfo?buildID=237494

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