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.
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.