[Bug 484676] Review Request: eclipse-dtp - Eclipse Data Tools Platform

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


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


Andrew Overholt <overholt at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |overholt at redhat.com




--- Comment #1 from Andrew Overholt <overholt at redhat.com>  2009-02-09 10:31:51 EDT ---
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

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.