Bug 484676

A few minor things:

- please set the fedora-review flag to ?
- change the Requires: on java to be >= (or maybe '='?) 1.5.0
- I prefer to add a short name after dropins:
  %files
  %{eclipse_dropin} => %{eclipse_dropin}/dtp
- please add a comment above the sed line getting rid of the sun.misc.Compare
- should we add some comment(s) stating why we're only building the features we are?

And the rest of the review (lines beginning with X need attention; those beginning with * are okay):

X verify the final provides and requires of the binary RPMs
  - other than the java one, things look good
X make sure lines are <= 80 characters
  - could you add some line continuations to fix this?
X package successfully compiles and builds
  - is this expected?

    [javac] 4. ERROR in /home/overholt/rpmbuild/BUILD/dtp-1.6.1/build/plugins/org.eclipse.datatools.connectivity.oda.design/src/org/eclipse/datatools/connectivity/oda/design/impl/InputElementUIHintsImpl.java
    [javac]  (at line 112)
    [javac] 	assert (eContainer() instanceof InputElementAttributes);
    [javac] 	^^^^^^
    [javac] The method assert(boolean) is undefined for the type InputElementUIHintsImpl

* BuildRequires are proper
* macros fine
* package is named appropriately
* it is legal for Fedora to distribute this
* license field matches the actual license.
* license is open source-compatible.
* specfile name matches %{name}
* md5sum matches upstream
  - not applicable
  - other than timestamps, my generated tarball matches the one in the SRPM
* skim the summary and description for typos, etc.
* summary and description good
  - the description is a bit vague but it is what upstream provides, so ...
* correct buildroot
* %{?dist} used correctly
* license text included in package and marked with %doc
* packages meets FHS (http://www.pathname.com/fhs/)
* rpmlint on <this package>.srpm gives no output
* changelog format okay
* Summary tag does not end in a period
* no PreReq
* specfile is legible
* specfile written in American English
* no -doc sub-package necessary
* not native, so no rpath, static linking, etc.
* no config files
* not a GUI app
* no -devel necessary
* install section begins with rm -rf $RPM_BUILD_ROOT or %{buildroot}
* no translations so no locale handling
* no Requires(pre,post)
* package not relocatable
* package contains code
* package owns all directories and files
* no %files duplicates
* file permissions fine
* %clean present
* %doc files do not affect runtime
* not a web app
* run rpmlint on the binary RPMs => no output
* package includes license text in the package and marks it with %doc

These minor things still stand:

(In reply to comment #1)
> - change the Requires: on java to be >= (or maybe '='?) 1.5.0
> - I prefer to add a short name after dropins:
>   %files
>   %{eclipse_dropin} => %{eclipse_dropin}/dtp
> - please add a comment above the sed line getting rid of the sun.misc.Compare
> - should we add some comment(s) stating why we're only building the features we
> are?

This last point is important.  Since we're not shipping any of the user-visible features, we need to stress this.  Ideally we'd have a bug for building all of DTP and then dependent bugs for any packages we need and/or modifications we need in other packages.

> X verify the final provides and requires of the binary RPMs
>   - other than the java one, things look good
> X make sure lines are <= 80 characters
>   - could you add some line continuations to fix this?

* package successfully compiles and builds

I evidently needed the rawhide SDK to build this.  We'll have to do an update if we want this to be in Fedora 10, I guess.  It all builds and installs (and is present after installation) fine now.

Spec URL:
http://akurtakov.fedorapeople.org/eclipse-dtp.spec
SRPM URL:
http://akurtakov.fedorapeople.org/eclipse-dtp-1.6.1-2.fc10.src.rpm



(In reply to comment #1)
> A few minor things:
> 
> - please set the fedora-review flag to ?
> - change the Requires: on java to be >= (or maybe '='?) 1.5.0
Fixed.

> - I prefer to add a short name after dropins:
>   %files
>   %{eclipse_dropin} => %{eclipse_dropin}/dtp
Fixed.

> - please add a comment above the sed line getting rid of the sun.misc.Compare
Fixed.

> - should we add some comment(s) stating why we're only building the features we
> are?
Fixed.

> 
> And the rest of the review (lines beginning with X need attention; those
> beginning with * are okay):
> 
> X verify the final provides and requires of the binary RPMs
>   - other than the java one, things look good
Fixed.

> X make sure lines are <= 80 characters
>   - could you add some line continuations to fix this?
Fixed wherever possible some paths are just too long.

> X package successfully compiles and builds
>   - is this expected?
> 
>     [javac] 4. ERROR in
> /home/overholt/rpmbuild/BUILD/dtp-1.6.1/build/plugins/org.eclipse.datatools.connectivity.oda.design/src/org/eclipse/datatools/connectivity/oda/design/impl/InputElementUIHintsImpl.java
>     [javac]  (at line 112)
>     [javac]  assert (eContainer() instanceof InputElementAttributes);
>     [javac]  ^^^^^^
>     [javac] The method assert(boolean) is undefined for the type
> InputElementUIHintsImpl
> 
Fixed.

Thanks for the updates.  The only thing I'd like to see is a comment at the beginning of the specfile stating which features we're building and which ones we're not and reasons for that.  Please add that, but since it's not a huge issue, I'll approve this now.

I've started a wiki page that we can use to track features/plugins we'd like and their dependencies.  We can add bug links to the wiki page as we file bugs.

https://fedoraproject.org/wiki/Eclipse#Plug-ins_We.27d_Like_To_Ship

New Package CVS Request
=======================
Package Name: eclipse-dtp
Short Description:  Eclipse Data Tools Platform
Owners: akurtakov
Branches: 
InitialCC: akurtakov

cvs done.

Packages available in rawhide now.