Bugzilla will be upgraded to version 5.0. The upgrade date is tentatively scheduled for 2 December 2018, pending final testing and feedback.
Bug 484676 - Review Request: eclipse-dtp - Eclipse Data Tools Platform
Review Request: eclipse-dtp - Eclipse Data Tools Platform
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Andrew Overholt
Fedora Extras Quality Assurance
:
Depends On: 477870
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-09 09:29 EST by Alexander Kurtakov
Modified: 2009-02-16 09:37 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-16 09:37:59 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
overholt: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Alexander Kurtakov 2009-02-09 09:29:25 EST
Spec URL:
http://akurtakov.fedorapeople.org/eclipse-dtp.spec
SRPM URL:
http://akurtakov.fedorapeople.org/eclipse-dtp-1.6.1-1.fc10.src.rpm

Description: The Eclipse Data Tools Platform provides extensible frameworks and exemplary tools, enabling a diverse set of plug-in offerings specific to particular data-centric technologies and supported by the DTP ecosystem.
Comment 1 Andrew Overholt 2009-02-09 10:31:51 EST
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
Comment 2 Andrew Overholt 2009-02-10 12:15:13 EST
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.
Comment 3 Alexander Kurtakov 2009-02-11 15:47:28 EST
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.
Comment 4 Andrew Overholt 2009-02-11 17:10:44 EST
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
Comment 5 Alexander Kurtakov 2009-02-12 02:04:19 EST
New Package CVS Request
=======================
Package Name: eclipse-dtp
Short Description:  Eclipse Data Tools Platform
Owners: akurtakov
Branches: 
InitialCC: akurtakov
Comment 6 Kevin Fenzi 2009-02-13 01:57:48 EST
cvs done.
Comment 7 Alexander Kurtakov 2009-02-16 09:37:59 EST
Packages available in rawhide now.

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