Bug 269421 - Review Request: eclipse-egit - Eclipse Git plugin
Review Request: eclipse-egit - Eclipse Git plugin
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Andrew Overholt
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-30 17:19 EDT by Ben Konrath
Modified: 2007-11-30 17:12 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-09-17 12:47:12 EDT
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 Ben Konrath 2007-08-30 17:19:02 EDT
Spec URL: 

http://bagu.org/eclipse/eclipse-egit.spec

SRPM URL: 

http://bagu.org/eclipse/eclipse-egit-0.2.2-0.git20070826.fc8.src.rpm

Description: 

The eclipse-egit package contains Eclipse plugins for interacting with Git repositories.
Comment 1 Andrew Overholt 2007-08-31 15:14:45 EDT
I'll take this.  Things are generally pretty good.  There are just a few minor
things (lines beginning with NEEDS_FIX) and some questions (lines beginning with ?):

MUST items:

OK package is named appropriately
OK is it legal for Fedora to distribute this?
? license field matches the actual license.
 - it says in the git web repo that some of it is LGPL ... but I can't see
   what parts - can you?  I'm okay with the dual GPLv2 and EPL as that's what
   I can see.
OK license is open source-compatible.
OK specfile name matches %{name}
? verify source and patches (md5sum matches upstream, know what the patches do)
 - I can't get the same md5sum but the contents are the same.  Did you use wget?
OK skim the summary and description for typos, etc.
OK correct buildroot
OK if %{?dist} is used, it should be in that form (note the ? and % locations)
OK license text included in package and marked with %doc
 - license text included in jar so can't mark as %doc
OK packages meets FHS (http://www.pathname.com/fhs/)
OK rpmlint on <this package>.srpm gives no output
 $ rpmlint eclipse-egit-0.2.2-0.git20070826.fc8.src.rpm 
 W: eclipse-egit invalid-license EPL GPLv2

 - this is fine since it's dual-licensed software

OK changelog should be in one of these formats:
 [...]
OK Packager tag should not be used
OK Vendor tag should not be used
OK Distribution tag should not be used
OK use License and not Copyright 
OK Summary tag should not end in a period
OK if possible, replace PreReq with Requires(pre) and/or Requires(post)
OK specfile is legible
OK package successfully compiles and builds on at least x86
OK BuildRequires are proper
OK summary should be a short and concise description of the package
OK description expands upon summary (don't include installation instructions)
OK make sure lines are <= 80 characters
 - they are, where possible
OK specfile written in American English
OK make a -doc sub-package if necessary
OK packages including libraries should exclude static libraries if possible
OK don't use rpath
OK config files should usually be marked with %config(noreplace)
OK GUI apps should contain .desktop files
OK should the package contain a -devel sub-package?
OK use macros appropriately and consistently
OK don't use %makeinstall
OK install section must begin with rm -rf $RPM_BUILD_ROOT or %{buildroot}
OK locale data handling correct (find_lang)
OK consider using cp -p to preserve timestamps
OK split Requires(pre,post) into two separate lines
OK package should probably not be relocatable
OK package contains code
OK package should own all directories and files
OK there should be no %files duplicates
OK file permissions should be okay; %defattrs should be present
NEEDS_FIX %clean should be present
 - you have ${RPM_BUILD_ROOT} and $RPM_BUILD_ROOT elsewhere
OK %doc files should not affect runtime
OK if it is a web app, it should be in /usr/share/%{name} and *not* /var/www
NEEDS_FIX verify the final provides and requires of the binary RPMs

  - do we need a Requires on eclipse-platform?

  $ rpm -qp --requires
../RPMS/x86_64/eclipse-egit-0.2.2-0.git20070826.fc8.x86_64.rpm  
  /usr/bin/rebuild-gcj-db  
  /usr/bin/rebuild-gcj-db  
  java-1.5.0-gcj >= 1.5.0
  java-1.5.0-gcj >= 1.5.0
  libc.so.6()(64bit)  
  libc.so.6(GLIBC_2.2.5)(64bit)  
  libdl.so.2()(64bit)  
  libgcc_s.so.1()(64bit)  
  libgcc_s.so.1(GCC_3.0)(64bit)  
  libgcj_bc.so.1()(64bit)  
  libm.so.6()(64bit)  
  libpthread.so.0()(64bit)  
  librt.so.1()(64bit)  
  libz.so.1()(64bit)  
  rpmlib(CompressedFileNames) <= 3.0.4-1
  rpmlib(PayloadFilesHavePrefix) <= 4.0-1
  rtld(GNU_HASH) 

  $ rpm -qp --provides
../RPMS/x86_64/eclipse-egit-0.2.2-0.git20070826.fc8.x86_64.rpm
  org.spearce.egit.core_0.2.2.200708311149.jar.so()(64bit)  
  org.spearce.egit.ui_0.2.2.200708311149.jar.so()(64bit)  
  org.spearce.jgit_0.2.2.200708311149.jar.so()(64bit)  
  eclipse-egit = 0.2.2-0.git20070826.fc8

NEEDS_FIX run rpmlint on the binary RPMs

  $ rpmlint ../RPMS/x86_64/eclipse-egit-0.2.2-0.git20070826.fc8.x86_64.rpm 
  W: eclipse-egit no-documentation
   - okay
  W: eclipse-egit incoherent-version-in-changelog 0.2.2-1.fc8
0.2.2-0.git20070826.fc8
   - please fix
  W: eclipse-egit invalid-license EPL GPLv2
   - this is fine ... unless we discover some LGPL stuff

SHOULD items:

OK package should include license text in the package and mark it with %doc
 - fine
? package should build on i386
 - it builds on x86_64 :)
OK package should build in mock
NEEDS_FIX we should probably fill in some of feature.xml such as the licence section
? should there be any user-visible eclipse features other than Team->Share?
  No checkout?  I know you said they were making a new release soon with a
  whole bunch of new features so should we wait until then?  I'm legitimately
  asking, not trying to be snide.  I notice a lot of stuff being spewed to the
  console as well ... do they have a bug tracker upstream?  I guess what I'm
  trying to say is that we shouldn't have it be installed by default in the
  Eclipse group of comps.xml just yet.  What do you think?
? should we split the package into two:  the java git implementation and the
  eclipse plugin?  I guess we can do that in the future if anything else
  starts using the java git implementation
Comment 2 Till Maas 2007-09-06 08:06:17 EDT
Andrew, please set fedora-review flag to ? to show that you will review this.
Comment 3 Ben Konrath 2007-09-06 14:09:05 EDT
New files:

http://bagu.org/eclipse/eclipse-egit.spec
http://bagu.org/eclipse/eclipse-egit-0.2.2-0.git20070826.fc8.src.rpm

(In reply to comment #1)
> I'll take this.  Things are generally pretty good.  There are just a few minor
> things (lines beginning with NEEDS_FIX) and some questions (lines beginning
with ?):
> 
> MUST items:
> 
> OK package is named appropriately
> OK is it legal for Fedora to distribute this?
> ? license field matches the actual license.
>  - it says in the git web repo that some of it is LGPL ... but I can't see
>    what parts - can you?  I'm okay with the dual GPLv2 and EPL as that's what
>    I can see.

The tests are LGPL but we're not shipping them. Should I add LGPL to the License
line because it's included in the src.rpm?

> OK license is open source-compatible.
> OK specfile name matches %{name}
> ? verify source and patches (md5sum matches upstream, know what the patches do)
>  - I can't get the same md5sum but the contents are the same.  Did you use wget?

No, wget doesn't work with the git web repo. I manually clicked on the link to
get the file.

> OK skim the summary and description for typos, etc.
> OK correct buildroot
> OK if %{?dist} is used, it should be in that form (note the ? and % locations)
> OK license text included in package and marked with %doc
>  - license text included in jar so can't mark as %doc
> OK packages meets FHS (http://www.pathname.com/fhs/)
> OK rpmlint on <this package>.srpm gives no output
>  $ rpmlint eclipse-egit-0.2.2-0.git20070826.fc8.src.rpm 
>  W: eclipse-egit invalid-license EPL GPLv2
> 
>  - this is fine since it's dual-licensed software
> 
> OK changelog should be in one of these formats:
>  [...]
> OK Packager tag should not be used
> OK Vendor tag should not be used
> OK Distribution tag should not be used
> OK use License and not Copyright 
> OK Summary tag should not end in a period
> OK if possible, replace PreReq with Requires(pre) and/or Requires(post)
> OK specfile is legible
> OK package successfully compiles and builds on at least x86
> OK BuildRequires are proper
> OK summary should be a short and concise description of the package
> OK description expands upon summary (don't include installation instructions)
> OK make sure lines are <= 80 characters
>  - they are, where possible
> OK specfile written in American English
> OK make a -doc sub-package if necessary
> OK packages including libraries should exclude static libraries if possible
> OK don't use rpath
> OK config files should usually be marked with %config(noreplace)
> OK GUI apps should contain .desktop files
> OK should the package contain a -devel sub-package?
> OK use macros appropriately and consistently
> OK don't use %makeinstall
> OK install section must begin with rm -rf $RPM_BUILD_ROOT or %{buildroot}
> OK locale data handling correct (find_lang)
> OK consider using cp -p to preserve timestamps
> OK split Requires(pre,post) into two separate lines
> OK package should probably not be relocatable
> OK package contains code
> OK package should own all directories and files
> OK there should be no %files duplicates
> OK file permissions should be okay; %defattrs should be present
> NEEDS_FIX %clean should be present
>  - you have ${RPM_BUILD_ROOT} and $RPM_BUILD_ROOT elsewhere

Fixed.

> OK %doc files should not affect runtime
> OK if it is a web app, it should be in /usr/share/%{name} and *not* /var/www
> NEEDS_FIX verify the final provides and requires of the binary RPMs
> 
>   - do we need a Requires on eclipse-platform?

Yes, it should have that. Fixed.

>   $ rpm -qp --requires
> ../RPMS/x86_64/eclipse-egit-0.2.2-0.git20070826.fc8.x86_64.rpm  
>   /usr/bin/rebuild-gcj-db  
>   /usr/bin/rebuild-gcj-db  
>   java-1.5.0-gcj >= 1.5.0
>   java-1.5.0-gcj >= 1.5.0
>   libc.so.6()(64bit)  
>   libc.so.6(GLIBC_2.2.5)(64bit)  
>   libdl.so.2()(64bit)  
>   libgcc_s.so.1()(64bit)  
>   libgcc_s.so.1(GCC_3.0)(64bit)  
>   libgcj_bc.so.1()(64bit)  
>   libm.so.6()(64bit)  
>   libpthread.so.0()(64bit)  
>   librt.so.1()(64bit)  
>   libz.so.1()(64bit)  
>   rpmlib(CompressedFileNames) <= 3.0.4-1
>   rpmlib(PayloadFilesHavePrefix) <= 4.0-1
>   rtld(GNU_HASH) 
> 
>   $ rpm -qp --provides
> ../RPMS/x86_64/eclipse-egit-0.2.2-0.git20070826.fc8.x86_64.rpm
>   org.spearce.egit.core_0.2.2.200708311149.jar.so()(64bit)  
>   org.spearce.egit.ui_0.2.2.200708311149.jar.so()(64bit)  
>   org.spearce.jgit_0.2.2.200708311149.jar.so()(64bit)  
>   eclipse-egit = 0.2.2-0.git20070826.fc8
> 
> NEEDS_FIX run rpmlint on the binary RPMs
> 
>   $ rpmlint ../RPMS/x86_64/eclipse-egit-0.2.2-0.git20070826.fc8.x86_64.rpm 
>   W: eclipse-egit no-documentation
>    - okay
>   W: eclipse-egit incoherent-version-in-changelog 0.2.2-1.fc8
> 0.2.2-0.git20070826.fc8
>    - please fix

Fixed.

>   W: eclipse-egit invalid-license EPL GPLv2
>    - this is fine ... unless we discover some LGPL stuff
> 
> SHOULD items:
> 
> OK package should include license text in the package and mark it with %doc
>  - fine
> ? package should build on i386
>  - it builds on x86_64 :)
> OK package should build in mock
> NEEDS_FIX we should probably fill in some of feature.xml such as the licence
section

I added the information that I could. This patch really needs to be upstream so
that this information can be filled in properly.

> ? should there be any user-visible eclipse features other than Team->Share?
>   No checkout?  I know you said they were making a new release soon with a
>   whole bunch of new features so should we wait until then?  I'm legitimately
>   asking, not trying to be snide.  

IMO this plugin needs a lot of work to be functional. I asked one of the
developers about their timeline but haven't received a reply yet.

> I notice a lot of stuff being spewed to the
>   console as well ... do they have a bug tracker upstream?  

No, not that I know of.

> I guess what I'm
>   trying to say is that we shouldn't have it be installed by default in the
>   Eclipse group of comps.xml just yet.  What do you think?

That seems reasonable.

> ? should we split the package into two:  the java git implementation and the
>   eclipse plugin?  I guess we can do that in the future if anything else
>   starts using the java git implementation

Yeah, I think it should be kept together until something needs it.
Comment 4 Andrew Overholt 2007-09-10 09:52:55 EDT
See my comments below.  The only thing is that I think we should either:

a) strip the tests from the SRPM to avoid worrying about the LGPL issue

or

b) add LGPL to the License line in the specfile and/or consult with legal to see
if it should or should not be there.

Otherwise, things are good to go.  Thanks for doing this, Ben.

(In reply to comment #3)
> (In reply to comment #1)
> > ? license field matches the actual license.
> >  - it says in the git web repo that some of it is LGPL ... but I can't see
> >    what parts - can you?  I'm okay with the dual GPLv2 and EPL as that's what
> >    I can see.
> 
> The tests are LGPL but we're not shipping them. Should I add LGPL to the License
> line because it's included in the src.rpm?

I really have no idea.  Let's strip them from the SRPM so we don't have to worry
about it.

> > ? verify source and patches (md5sum matches upstream, know what the patches do)
> >  - I can't get the same md5sum but the contents are the same.  Did you use wget?
> 
> No, wget doesn't work with the git web repo. I manually clicked on the link to
> get the file.

Hmm.  I used wget and just renamed the file.  There are no content differences,
though, so I'm okay with it.

> > NEEDS_FIX we should probably fill in some of feature.xml such as the licence
> section
> 
> I added the information that I could. This patch really needs to be upstream so
> that this information can be filled in properly.

Okay, that sounds fine.

> > ? should there be any user-visible eclipse features other than Team->Share?
> >   No checkout?  I know you said they were making a new release soon with a
> >   whole bunch of new features so should we wait until then?  I'm legitimately
> >   asking, not trying to be snide.  
> 
> IMO this plugin needs a lot of work to be functional. I asked one of the
> developers about their timeline but haven't received a reply yet.
> 
> > I notice a lot of stuff being spewed to the
> >   console as well ... do they have a bug tracker upstream?  
> 
> No, not that I know of.
> 
> > I guess what I'm
> >   trying to say is that we shouldn't have it be installed by default in the
> >   Eclipse group of comps.xml just yet.  What do you think?
> 
> That seems reasonable.

Okay, I'm glad that it's going to be available so we can get it some exposure.

> > ? should we split the package into two:  the java git implementation and the
> >   eclipse plugin?  I guess we can do that in the future if anything else
> >   starts using the java git implementation
> 
> Yeah, I think it should be kept together until something needs it.

Agreed.
Comment 5 Till Maas 2007-09-10 10:32:03 EDT
(In reply to comment #4)

> a) strip the tests from the SRPM to avoid worrying about the LGPL issue

Afaik does the License Tag refer to the shipped binaries, this should be on
mentioned the -devel list .
Comment 6 Ben Konrath 2007-09-10 12:51:47 EDT
Andrew, Let me know what needs to be done wrt the License tag - I have no
complaints either way. Thanks, Ben
Comment 7 Andrew Overholt 2007-09-10 13:52:29 EDT
Let's leave the license as-is (GPLv2, EPL) and leave the source tarball as-is as
well.

Approved.
Comment 8 Ben Konrath 2007-09-14 15:05:34 EDT
New Package CVS Request
=======================
Package Name: eclipse-egit
Short Description: Eclipse Git plugin
Owners: bkonrath
Branches: F-7
InitialCC:
Cvsextras Commits: yes
Comment 9 Kevin Fenzi 2007-09-14 17:27:50 EDT
cvs done.

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