Bug 507958 - Review Request: eclipse-rse - Eclipse Remote System Explorer framework
Summary: Review Request: eclipse-rse - Eclipse Remote System Explorer framework
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Andrew Overholt
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 252223 (view as bug list)
Depends On: 507693 507710
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-06-24 20:35 UTC by Jeff Johnston
Modified: 2009-07-28 13:57 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-07-23 21:50:55 UTC
Type: ---
Embargoed:
overholt: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Jeff Johnston 2009-06-24 20:35:50 UTC
Spec URL: ftp://sources.redhat.com/pub/newlib/eclipse-rse.spec
SRPM URL: ftp://sources.redhat.com/pub/newlib/eclipse-rse-3.0.3-1.fc12.src.rpm
Description: Remote System Explorer (RSE) is a framework and toolkit in Eclipse
 Workbench that allows you to connect and work with a variety of remote systems.  It is required by the latest CDT release 6.0.

Comment 1 Andrew Overholt 2009-06-29 17:54:47 UTC
Thanks for the submission, Jeff.  A few comments:

- I think you're missing Requires on eclipse-cdt and eclipse-emf
- will we move to 3.1 once we get a Galileo CDT build?
- please use either %{buildroot} or $RPM_BUILD_ROOT but not both
- we should perhaps have Eclipse in the Summary field
- I think "framework" can be dropped from the Summary field
- please change the permissions on the fetch script to avoid this rpmlint warning:

eclipse-rse.src: W: strange-permission fetch-rse.sh 0775

- you should probably mark a top-level feature's about.html as %doc
- you should look for @build@ and replace it with the same qualifier that upstream uses.  In fact, you should use forceContextQualifier (see eclipse-mylyn.spec for an example) to ensure our versions are the same as upstream's for 3.0.3

The package builds for me, follows the packaging guidelines, and functions in Eclipse.  Thanks for the high quality submission.  Once the minor issues above are cleaned up, I will approve this.

Comment 2 Jeff Johnston 2009-07-20 21:16:46 UTC
(In reply to comment #1)
> Thanks for the submission, Jeff.  A few comments:
> 
> - I think you're missing Requires on eclipse-cdt and eclipse-emf

done

> - will we move to 3.1 once we get a Galileo CDT build?

Yes.  CDT 6.0 only requires RSE >= 3.0.

> - please use either %{buildroot} or $RPM_BUILD_ROOT but not both

done

> - we should perhaps have Eclipse in the Summary field

done

> - I think "framework" can be dropped from the Summary field

ok, done

> - please change the permissions on the fetch script to avoid this rpmlint
> warning:
> 
> eclipse-rse.src: W: strange-permission fetch-rse.sh 0775
> 

done

> - you should probably mark a top-level feature's about.html as %doc

done.  I get a warning about file being specified twice, but I don't see any other examples in any of the other eclipse projects.

> - you should look for @build@ and replace it with the same qualifier that
> upstream uses.  In fact, you should use forceContextQualifier (see
> eclipse-mylyn.spec for an example) to ensure our versions are the same as
> upstream's for 3.0.3
> 

I can't use forceContextQualifier because the versions are being set by map files.  The RSE project has many version qualifiers for the various plugins and features.  It does not create a single build id for a release and none of the qualifiers are filled in for the various tags.  I have fixed this by using the map files to a) fetch the plugins/features for the tarball b) create a featureVersions.properties and pluginVersions.properties file from the same map files.  Both a and b are performed in the fetch-rse.sh script.  The build now matches the upstream version numbers, including the suffixes which are used for features.  I have opened an RFE for pdebuild to allow map files to be specified which should make this easier for future rpm packaging.

> The package builds for me, follows the packaging guidelines, and functions in
> Eclipse.  Thanks for the high quality submission.  Once the minor issues above
> are cleaned up, I will approve this.  

Thanks.

Comment 3 Jeff Johnston 2009-07-20 21:17:36 UTC
I forgot to mention that I have overwritten the spec file and SRPM:

Spec URL: ftp://sources.redhat.com/pub/newlib/eclipse-rse.spec
SRPM URL: ftp://sources.redhat.com/pub/newlib/eclipse-rse-3.0.3-1.fc12.src.rpm

Comment 4 Alexander Kurtakov 2009-07-21 13:42:25 UTC
(In reply to comment #2)
> > - you should probably mark a top-level feature's about.html as %doc
> 
> done.  I get a warning about file being specified twice, but I don't see any
> other examples in any of the other eclipse projects.
> 
You can directly use paths as they are inside sources and this will prevent this warnings from showing, e.g.:
%doc org.eclipse.rse.sdk-feature/epl-v10.html
%doc org.eclipse.rse.sdk-feature/license.html

Comment 5 Andrew Overholt 2009-07-21 14:50:28 UTC
A few more minor things:

- you have ${_jvmdir} and %{_jvmdir}
- this is weird:  $RPM_BUILD_ROOT%{install_loc} ... please use %{buildroot}%{install_loc}.  In fact, please s/$RPM_BUILD_ROOT/%{buildroot}/g in the whole file
- do what Alex suggests for the doc files

Thanks for dealing with the qualifier situation.

Comment 6 Jeff Johnston 2009-07-21 17:13:06 UTC
(In reply to comment #5)
> A few more minor things:
> 
> - you have ${_jvmdir} and %{_jvmdir}

fixed to use %{_jvmdir}

> - this is weird:  $RPM_BUILD_ROOT%{install_loc} ... please use
> %{buildroot}%{install_loc}.  In fact, please s/$RPM_BUILD_ROOT/%{buildroot}/g
> in the whole file

done

> - do what Alex suggests for the doc files
> 

done

> Thanks for dealing with the qualifier situation.  

Replaced files at:

Spec URL: ftp://sources.redhat.com/pub/newlib/eclipse-rse.spec
SRPM URL: ftp://sources.redhat.com/pub/newlib/eclipse-rse-3.0.3-1.fc12.src.rpm

Comment 7 Andrew Overholt 2009-07-21 17:41:31 UTC
Thanks.  This submission is approved.

Comment 8 Jeff Johnston 2009-07-21 18:43:16 UTC
New Package CVS Request
=======================
Package Name: eclipse-rse
Short Description: Eclipse Remote System Explorer
Owners: jjohnstn
Branches: 
InitialCC: jjohnstn

Comment 9 Jason Tibbitts 2009-07-23 16:35:46 UTC
CVS done.

Comment 10 Jeff Johnston 2009-07-23 21:50:55 UTC
eclipse-rse-3.0.3-1.fc12 successfully built

Comment 11 Alexander Kurtakov 2009-07-28 13:57:29 UTC
*** Bug 252223 has been marked as a duplicate of this bug. ***


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