I'll take this one.

Okay, first things first:  rpmlint output (on FC6):

W: eclipse-cdt non-standard-group Text Editors/Integrated Development
Environments (IDE)

This one I'm not sure about so let's leave it for now.  I'll look into it.

W: eclipse-cdt invalid-license Eclipse Public License - v 1.0 (EPL)
<http://www.eclipse.org/legal/epl-v10.html>

Just put EPL and not the version or the full name or the URL.

W: eclipse-cdt macro-in-%changelog eclipse_arch

Macros in %changelogs need to be double-%'d.  Change %{eclipse_arch} to
%%{eclipse_arch}.

W: eclipse-cdt mixed-use-of-spaces-and-tabs (spaces: line 7, tab: line 3)

Open the specfile in emacs and M-x untabify.

And now some comments about the specfile:

. don't use pkg_summary.  just put the summary in Summary:
. I don't think we need eclipse_name.  just replace that with eclipse in its 3 uses.
. get rid of the section macro
. I hate that there's an epoch but there's nothing we can do about that now
. arch-specific plugins such as org.eclipse.cdt.core.linux should be moved to
%{_libdir}/eclipse
. does the CDT still use ctags?
. do any of the jars contain arch-specific bits (.sos, etc.) that may make it
multilib-incompatible?
. eclipse_lib_base isn't currently used but it will be when you move the
arch-specific plugins there
. I think the instructions for generating the tarball no longer hold. 
Specifically, I think it should now be:

eclipse -Duser.home=../../home -application <everything else>
. is the autotools stuff all licensed properly?  ie. it's all EPL and it all has
the correct copyright notices in the files?
. could we add comments for all of the patches?  It would greatly help figuring
out why we're patching and what each patch is doing.
. is CPPUnit support EPL?
. should we require gcc?  what about gcc-c++?  Perhaps gdb and/or make already
require those ...
. can we look at adding all of the arches?  or at least can we add a comment
specifying why we're only building on the 4 we are?
. the sdk's %description is weak.  look at the sdk %descriptions in eclipse.spec
. we shouldn't have links between /usr/share/eclipse and /usr/lib/eclipse for
the .sos.  Ben, what do you think about this one?

(In reply to comment #2)
> Okay, first things first:  rpmlint output (on FC6):
> 
> W: eclipse-cdt non-standard-group Text Editors/Integrated Development
> Environments (IDE)
> 
> This one I'm not sure about so let's leave it for now.  I'll look into it.
> 
> W: eclipse-cdt invalid-license Eclipse Public License - v 1.0 (EPL)
> <http://www.eclipse.org/legal/epl-v10.html>
> 
> Just put EPL and not the version or the full name or the URL.
> 
> W: eclipse-cdt macro-in-%changelog eclipse_arch
> 
> Macros in %changelogs need to be double-%'d.  Change %{eclipse_arch} to
> %%{eclipse_arch}.
> 
> W: eclipse-cdt mixed-use-of-spaces-and-tabs (spaces: line 7, tab: line 3)
> 
> Open the specfile in emacs and M-x untabify.

New SRPM URL: http://www.vermillionskye.com/eclipse-cdt-3.1.1-6.1.fc7.src.rpm

All comments above handled.

(In reply to comment #3)
> And now some comments about the specfile:
> 
> . don't use pkg_summary.  just put the summary in Summary:

Done.

> . I don't think we need eclipse_name.  just replace that with eclipse in its 3
uses.

Done.

> . get rid of the section macro

Done.

> . I hate that there's an epoch but there's nothing we can do about that now
> . arch-specific plugins such as org.eclipse.cdt.core.linux should be moved to
> %{_libdir}/eclipse

Done.

> . does the CDT still use ctags?

There is a comment it has been removed so ctags requirement removed.

> . do any of the jars contain arch-specific bits (.sos, etc.) that may make it
> multilib-incompatible?

Not sure what you mean other than the arch plug-ins.

> . eclipse_lib_base isn't currently used but it will be when you move the
> arch-specific plugins there

Yes, it used now.

> . I think the instructions for generating the tarball no longer hold. 
> Specifically, I think it should now be:
> 
> eclipse -Duser.home=../../home -application <everything else>

Yes.  Comments added and checked for all tarballs used.

> . is the autotools stuff all licensed properly?  ie. it's all EPL and it all has
> the correct copyright notices in the files?

It does now.  Comments added and licenses added.  Source tarball updated.

> . could we add comments for all of the patches?  It would greatly help figuring
> out why we're patching and what each patch is doing.

Done.

> . is CPPUnit support EPL?

No.  It is CPL.

> . should we require gcc?  what about gcc-c++?  Perhaps gdb and/or make already
> require those ...

Yes, I believe we should.  We use parts of gcc.

> . can we look at adding all of the arches?  or at least can we add a comment
> specifying why we're only building on the 4 we are?

I have added a comment.

> . the sdk's %description is weak.  look at the sdk %descriptions in eclipse.spec

Done.

> . we shouldn't have links between /usr/share/eclipse and /usr/lib/eclipse for
> the .sos.  Ben, what do you think about this one?

Nothing done on this.  Let me know what is needed.

(In reply to comment #5)
> (In reply to comment #3)
> > . we shouldn't have links between /usr/share/eclipse and /usr/lib/eclipse for
> > the .sos.  Ben, what do you think about this one?
> 
> Nothing done on this.  Let me know what is needed.
 
For FHS compliance you need to put fragments with shared libraries in
%{libdir}/eclipse/plugins to avoid symlinking from /usr/share to /usr/lib. See
this document for more information:

http://wiki.eclipse.org/index.php/FHS_Compliant_Packages

Plugins with shared libraries are also plugins that get moved to
/usr/lib/eclipse already because of arch.  Thus, section in install that moves
.so and links them to /usr/share has been removed.

Specfile and srpm have been updated.

(In reply to comment #7)
> Specfile and srpm have been updated.

What are the new URLs?

Spec URL: http://www.vermillionskye.com/eclipse-cdt.spec
SRPM URL: http://www.vermillionskye.com/eclipse-cdt-3.1.1-6.1.fc7.src.rpm

I can't build that SRPM:

+ mkdir -p /var/tmp/eclipse-cdt-buildroot/usr/lib/eclipse/plugins
++ find ./usr/share/eclipse/plugins -name '*x86_3.1.1*'
find: ./usr/share/eclipse: No such file or directory
+ popd
/var/tmp/rpm-tmp.90082: line 40: popd: directory stack empty
error: Bad exit status from /var/tmp/rpm-tmp.90082 (%install)

My bad.  I removed the .so move stuff but clipped two lines that created and
pushed a directory.  The files have been updated (same URL).  In this round, I
also removed gcj .so files from the SDK package as they are already in the base rpm.

Spec URL: http://www.vermillionskye.com/eclipse-cdt.spec
SRPM URL: http://www.vermillionskye.com/eclipse-cdt-3.1.1-7.fc7.src.rpm

Almost there.  Just fix the lines beginning with an X:

MUST:
* rpmlint on eclipse-cdt srpm gives no output
* package is named appropriately
* specfile name matches %{name}
X package meets packaging guidelines.

BuildRoot incorrect.  As per this:

http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot

it should be:

%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

Also, you have %{dist} where that should be %{?dist}.

X license field matches the actual license.

because CPPUnit is still CPL, it has to be:

License: EPL/CPL

You need to speak to upstream to get them to re-license the cppunit plugin(s)

* license is open source-compatible.
X license text included in package and marked with %doc

you'll need to include a copy of epl-v10.html and cpl.html and mark them with
%doc in the files section.  Just put them in the cdt core and/or the cdt sdk
feature directories.

* specfile written in American English
* specfile is legible
* source files match upstream
  upstream doesn't provide a source tarball but instructions on how to
  generate are provided; I can't reproduce the tarballs exactly using these
  intructions (size differences), but a diff of the exploded contents gives
  nothing.
* package successfully compiles and builds on at least x86 (it's building on
the other arches in Fedora Core presently)

> please file a bug in Red Hat bugzilla to investigate building on all arches
> also, please file a bug with upstream regarding this; we don't care if they
> _provide_ builds on other platforms than they do now, but it should at least
> be buildable on all arches.

* BuildRequires are proper
* no locale data so no find_lang necessary
* package is not relocatable
* package owns all directories and files
* no %files duplicates
* file permissions are fine; %defattrs present
* %clean present
* macro usage is consistent
* package contains code
* no large docs so no -doc subpackage
  the doc plugins aren't usable outside of Eclipse so there's no point marking
  them as %doc
* %doc files don't affect runtime (N/A)
* shared libraries are present, but no ldconfig required.
* no pkgconfig or header files
* no -devel package
* no .la files
* no desktop file
* not a web app.
* file ownership fine
X final provides and requires are sane

$ rpm -qp --provides eclipse-cdt-3.1.1-7.i386.rpm
cdt_linux.jar.so  
com.redhat.eclipse.cdt.autotools_0.0.6.jar.so  
cppunit.jar.so  
libpty.so  
libspawner.so  
org.eclipse.cdt.core_3.1.1.200701181121.jar.so  
org.eclipse.cdt.debug.core_3.1.1.200701181121.jar.so  
org.eclipse.cdt.debug.mi.core_3.1.1.200701181121.jar.so  
org.eclipse.cdt.debug.mi.ui_3.1.1.200701181121.jar.so  
org.eclipse.cdt.debug.ui_3.1.1.200701181121.jar.so  
org.eclipse.cdt.launch_3.1.1.200701181121.jar.so  
org.eclipse.cdt.make.core_3.1.1.200701181121.jar.so  
org.eclipse.cdt.make.ui_3.1.1.200701181121.jar.so  
org.eclipse.cdt.managedbuilder.core_3.1.1.200701181121.jar.so  
org.eclipse.cdt.managedbuilder.gnu.ui_3.1.1.200701181121.jar.so  
org.eclipse.cdt.managedbuilder.ui_3.1.1.200701181121.jar.so  
org.eclipse.cdt.refactoring_3.1.1.200701181121.jar.so  
org.eclipse.cdt.ui_3.1.1.200701181121.jar.so  
eclipse-cdt = 1:3.1.1-7

$ rpm -qp --provides eclipse-cdt-sdk-3.1.1-7.i386.rpm
eclipse-cdt-sdk = 1:3.1.1-7

$ rpm -qp --requires eclipse-cdt-3.1.1-7.i386.rpm
/bin/sh  
/bin/sh  
eclipse-platform  
eclipse-platform >= 1:3.2.0
gdb  
java-gcj-compat >= 1.0.64
java-gcj-compat >= 1.0.64
libc.so.6  
libc.so.6(GLIBC_2.0)  
libc.so.6(GLIBC_2.1)  
libc.so.6(GLIBC_2.1.3)  
libdl.so.2  
libgcc_s.so.1  
libgcc_s.so.1(GCC_3.0)  
libgcc_s.so.1(GLIBC_2.0)  
libgcj_bc.so.1  
libm.so.6  
libpthread.so.0  
librt.so.1  
libz.so.1  
make  
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rtld(GNU_HASH) 

X do we need a Requirement on gcc?

$ rpm -qp --requires eclipse-cdt-sdk-3.1.1-7.i386.rpm
eclipse-cdt = 1:3.1.1-7
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1

SHOULD:
X package includes license text
  see my comments above about including the EPL and CPL texts as html in the
  feature directories
* package builds on i386
  ... and others in brew ATM; I don't envision a problem here)
* package functions in Eclipse
X package builds in mock
  my mock setup doesn't seem to be working but I don't anticipate any problems
  here as the package currently builds fine in brew

Packages updated at same URLs.

(In reply to comment #12)
> Almost there.  Just fix the lines beginning with an X:
> 
> MUST:
> * rpmlint on eclipse-cdt srpm gives no output
> * package is named appropriately
> * specfile name matches %{name}
> X package meets packaging guidelines.
> 
> BuildRoot incorrect.  As per this:
> 
> http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot
> 
> it should be:
> 
> %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
> 

Done.

> Also, you have %{dist} where that should be %{?dist}.
>

Done.
 
> X license field matches the actual license.
> 
> because CPPUnit is still CPL, it has to be:
> 
> License: EPL/CPL
>

Have changed to: Eclipse Public License / CPL

rpmlint doesn't like the combination, but individually, each license above
is valid to rpmlint.
 
> You need to speak to upstream to get them to re-license the cppunit plugin(s)
> 
> * license is open source-compatible.
> X license text included in package and marked with %doc
> 
> you'll need to include a copy of epl-v10.html and cpl.html and mark them with
> %doc in the files section.  Just put them in the cdt core and/or the cdt sdk
> feature directories.
> 

Done.

> * specfile written in American English
> * specfile is legible
> * source files match upstream
>   upstream doesn't provide a source tarball but instructions on how to
>   generate are provided; I can't reproduce the tarballs exactly using these
>   intructions (size differences), but a diff of the exploded contents gives
>   nothing.
> * package successfully compiles and builds on at least x86 (it's building on
> the other arches in Fedora Core presently)
> 
> > please file a bug in Red Hat bugzilla to investigate building on all arches
> > also, please file a bug with upstream regarding this; we don't care if they
> > _provide_ builds on other platforms than they do now, but it should at least
> > be buildable on all arches.
> 
> * BuildRequires are proper
> * no locale data so no find_lang necessary
> * package is not relocatable
> * package owns all directories and files
> * no %files duplicates
> * file permissions are fine; %defattrs present
> * %clean present
> * macro usage is consistent
> * package contains code
> * no large docs so no -doc subpackage
>   the doc plugins aren't usable outside of Eclipse so there's no point marking
>   them as %doc
> * %doc files don't affect runtime (N/A)
> * shared libraries are present, but no ldconfig required.
> * no pkgconfig or header files
> * no -devel package
> * no .la files
> * no desktop file
> * not a web app.
> * file ownership fine
> X final provides and requires are sane
> 
> $ rpm -qp --provides eclipse-cdt-3.1.1-7.i386.rpm
> cdt_linux.jar.so  
> com.redhat.eclipse.cdt.autotools_0.0.6.jar.so  
> cppunit.jar.so  
> libpty.so  
> libspawner.so  
> org.eclipse.cdt.core_3.1.1.200701181121.jar.so  
> org.eclipse.cdt.debug.core_3.1.1.200701181121.jar.so  
> org.eclipse.cdt.debug.mi.core_3.1.1.200701181121.jar.so  
> org.eclipse.cdt.debug.mi.ui_3.1.1.200701181121.jar.so  
> org.eclipse.cdt.debug.ui_3.1.1.200701181121.jar.so  
> org.eclipse.cdt.launch_3.1.1.200701181121.jar.so  
> org.eclipse.cdt.make.core_3.1.1.200701181121.jar.so  
> org.eclipse.cdt.make.ui_3.1.1.200701181121.jar.so  
> org.eclipse.cdt.managedbuilder.core_3.1.1.200701181121.jar.so  
> org.eclipse.cdt.managedbuilder.gnu.ui_3.1.1.200701181121.jar.so  
> org.eclipse.cdt.managedbuilder.ui_3.1.1.200701181121.jar.so  
> org.eclipse.cdt.refactoring_3.1.1.200701181121.jar.so  
> org.eclipse.cdt.ui_3.1.1.200701181121.jar.so  
> eclipse-cdt = 1:3.1.1-7
> 
> $ rpm -qp --provides eclipse-cdt-sdk-3.1.1-7.i386.rpm
> eclipse-cdt-sdk = 1:3.1.1-7
> 
> $ rpm -qp --requires eclipse-cdt-3.1.1-7.i386.rpm
> /bin/sh  
> /bin/sh  
> eclipse-platform  
> eclipse-platform >= 1:3.2.0
> gdb  
> java-gcj-compat >= 1.0.64
> java-gcj-compat >= 1.0.64
> libc.so.6  
> libc.so.6(GLIBC_2.0)  
> libc.so.6(GLIBC_2.1)  
> libc.so.6(GLIBC_2.1.3)  
> libdl.so.2  
> libgcc_s.so.1  
> libgcc_s.so.1(GCC_3.0)  
> libgcc_s.so.1(GLIBC_2.0)  
> libgcj_bc.so.1  
> libm.so.6  
> libpthread.so.0  
> librt.so.1  
> libz.so.1  
> make  
> rpmlib(CompressedFileNames) <= 3.0.4-1
> rpmlib(PayloadFilesHavePrefix) <= 4.0-1
> rtld(GNU_HASH) 
> 
> X do we need a Requirement on gcc?
> 

Per offline conversation, I have required gcc-c++, automake, and autoconf.

> $ rpm -qp --requires eclipse-cdt-sdk-3.1.1-7.i386.rpm
> eclipse-cdt = 1:3.1.1-7
> rpmlib(CompressedFileNames) <= 3.0.4-1
> rpmlib(PayloadFilesHavePrefix) <= 4.0-1
> 
> SHOULD:
> X package includes license text
>   see my comments above about including the EPL and CPL texts as html in the
>   feature directories

Ok.

> * package builds on i386
>   ... and others in brew ATM; I don't envision a problem here)
> * package functions in Eclipse
> X package builds in mock
>   my mock setup doesn't seem to be working but I don't anticipate any problems
>   here as the package currently builds fine in brew
> 

Ok.

APPROVED

Thanks, Jeff!

This package can't move to Extras until eclipse-changelog (bug #222365) does.

Hey Andrew. Can this be closed now? 
You might also set the fedora-review to + also to conform to the current rules... 
(although I don't think it matters too much either way)

Yes, we can close this now and set it to +.  Thanks for the reminder.