Bug 191017 - Review Request: eclipse-subclipse
Review Request: eclipse-subclipse
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On: 191014 191015
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-05-07 23:46 EDT by Robert Marcano
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-08-26 21:56:52 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
updated spec file (7.09 KB, text/plain)
2006-05-23 16:38 EDT, Ben Konrath
no flags Details
log from failed build (20.06 KB, text/plain)
2006-07-03 15:40 EDT, Jason Tibbitts
no flags Details

  None (edit)
Description Robert Marcano 2006-05-07 23:46:39 EDT
Spec URL: http://www.marcanoonline.com/downloads/fedora/package_submissions/subclipse/subclipse.spec
SRPM URL: http://www.marcanoonline.com/downloads/fedora/package_submissions/subclipse/subclipse-1.0.1-2.src.rpm
Description: Subclipse is an Eclipse plugin that adds Subversion integration to the Eclipse IDE
Comment 1 Andrew Overholt 2006-05-23 14:34:08 EDT
Is anyone opposed to following the convention of calling things
eclipse-<feature>?  I agree that in cases like subclipse it's a bit annoying but
we can add Provides: subclipse.  Thoughts?
Comment 2 Ben Konrath 2006-05-23 16:28:29 EDT
I think using eclipse-subclipse would be a good idea. We used eclipse-phpeclipse
in RHEL and it would be nice to be consistent with package names. 
Comment 3 Ben Konrath 2006-05-23 16:38:18 EDT
Created attachment 129890 [details]
updated spec file

Here's an updated the spec with the following changes:

changelog
* Tue May 23 2006 Ben Konrath <bkonrath@redhat.com> 1.0.1-3
- Rename package to eclispe-subclipse.
- Use copy-platform script for now.
Comment 5 Ben Konrath 2006-05-29 18:30:28 EDT
Nice work here. I'm in the process of making an update to Eclipse in FC5 that
will contain the package build stuff. I should have it out tomorrow or Wed. I'll
post a message here when it has been released. 
Comment 6 Ben Konrath 2006-06-01 13:45:19 EDT
Ok, the latest update to FC5 (3.1.2-1jpp_15fc) has the package buildstuff. This
message explains how to use it:

http://dev.eclipse.org/mhonarc/lists/linux-distros-dev/msg00010.html

Let me know if you have any questions or want me to update the spec file. Thanks.
Comment 8 Ben Konrath 2006-06-04 23:33:44 EDT
I just filed bug #194026 for the javahl problem. I'll investigate as soon as I
get time.
Comment 9 Ben Konrath 2006-06-14 00:34:32 EDT
We may have lost some useful comments here. It would be great if people could
re-post anything still relevant. Thanks.
Comment 11 Jason Tibbitts 2006-06-26 16:28:37 EDT
I had to add --username guest in order to checkout the upstream source.  The
result, however, was not the same as what you're shipping in the spec.  No idea
what's going on there.

Need to head home now; more when I get there.
Comment 12 Robert Marcano 2006-06-26 17:30:55 EDT
ohhh I see the diferences that i find are the .svn/* files. for example

 <entry
    committed-rev="1423"
    name="toc.xml"
-   text-time="2006-06-26T21:31:32.000000Z"
+   text-time="2006-06-26T02:41:55.000000Z"
    committed-date="2005-06-30T23:29:12.931206Z"
    checksum="6810cf546f38ea63597492c3b80a05fb"
    last-author="dlr"
    kind="file"
-   prop-time="2006-06-26T21:31:26.000000Z"/>
+   prop-time="2006-06-26T02:41:36.000000Z"/>

I will remove then before packaging
Comment 13 Jason Tibbitts 2006-06-26 17:58:00 EDT
It might be nice if you included a quick script which fetches and builds the
tarball you use.  This is commonly done for packages which must remove something
(like mp3 support) from the upstream tarball.
Comment 14 Jeffrey C. Ollie 2006-06-27 08:22:59 EDT
I would suggest using "svn export" rather than "svn checkout" to create the
tarball.  "svn export" will grab a copy of the code without creating the .svn
directories.
Comment 16 Robert Marcano 2006-07-02 23:08:24 EDT
Removing bug #191016 from the dependecies list
Comment 17 Jason Tibbitts 2006-07-03 15:39:28 EDT
I don't think it's going to be possible for a tarball I generated from an svn
export to have the same checksum as yours, since the timestamps of the
directories will always show the checkout time.  diff, however, doesn't find any
differences and that's good enough for me.

Unfortunately the package doesn't build in mock for me.  I get down to here:

+ java -cp /usr/share/eclipse/startup.jar
-Duser.home=/builddir/build/BUILD/subclipse-1.0.3/subclipse/home
org.eclipse.core.launcher.Main -application org.eclipse.ant.core.antRunner
-Dtype=feature -Did=org.tigris.subversion.subclipse
-DsourceDirectory=/builddir/build/BUILD/subclipse-1.0.3/subclipse
-DbaseLocation=/builddir/build/BUILD/subclipse-1.0.3/subclipse/SDK
-Dbuilder=/usr/share/eclipse/plugins/org.eclipse.pde.build_3.1.2/templates/package-build
-f /usr/share/eclipse/plugins/org.eclipse.pde.build_3.1.2/scripts/build.xml
Buildfile: /usr/share/eclipse/plugins/org.eclipse.pde.build_3.1.2/scripts/build.xml

BUILD FAILED
Buildfile:
/usr/share/eclipse/plugins/org.eclipse.pde.build_3.1.2/scripts/build.xml does
not exist

I'll attach my build.log; let me know if there's any other information I can
provide.
Comment 18 Jason Tibbitts 2006-07-03 15:40:25 EDT
Created attachment 131887 [details]
log from failed build
Comment 19 Andrew Overholt 2006-07-03 15:50:15 EDT
(In reply to comment #17)
> BUILD FAILED
> Buildfile:
> /usr/share/eclipse/plugins/org.eclipse.pde.build_3.1.2/scripts/build.xml does
> not exist

This is 'cause we updated to 3.2.0 in rawhide.  I forget if we kept the
versionless pde.build symlink in there -- I think we did -- but I can provide a
patch tomorrow.  Sorry about that.

Comment 20 Andrew Overholt 2006-07-04 10:11:22 EDT
(In reply to comment #19)
> (In reply to comment #17)
> > BUILD FAILED
> > Buildfile:
> > /usr/share/eclipse/plugins/org.eclipse.pde.build_3.1.2/scripts/build.xml does
> > not exist
> 
> This is 'cause we updated to 3.2.0 in rawhide.  I forget if we kept the
> versionless pde.build symlink in there -- I think we did -- but I can provide a
> patch tomorrow.

Patch:

http://www.overholt.ca/eclipse/eclipse-subclipse.spec.patch
Comment 21 Jason Tibbitts 2006-07-07 12:32:23 EDT
Did you want me to review this package with that patch in place, or should I
wait for you to cut a new package?
Comment 22 Robert Marcano 2006-07-07 13:07:18 EDT
I was planning to install rawhide on a VM this weekend, in order to test with
the added eclipse 3.2, to see if nothing breaks, specially my patch to disable
the native javahl interface to see if something fixs it in the current
incarnation of FC-6. but If you can review it with the patch, better
Comment 23 Ben Konrath 2006-07-27 16:43:59 EDT
It would be nice to get this pacakge reviewed with the patch in place. I'm still
planning to track down the problem, but it is is a *very* complex GCJ problem.
Also, I won't get time until mid August. Robert, are you ok with this?
Comment 24 Jason Tibbitts 2006-07-27 17:39:43 EDT
Unfortunately that patch doesn't apply to the spec in
http://www.marcanoonline.com/downloads/fedora/package_submissions/subclipse/eclipse-subclipse-1.0.3-2.src.rpm

I don't think I botched it when I applied it manually.
However, now there are dependency issues; ganymed-ssh2 and javasvn need rebuilds
against libgcj.so.7.

Is it reasonable to build against FC5 here?
Comment 25 Jason Tibbitts 2006-07-27 17:48:35 EDT
Never mind; ganymed-ssh2 isn't build for FC5.  So there's pretty much no way
this can be reviewed right now.
Comment 26 Ben Konrath 2006-07-28 15:48:34 EDT
Here's an updated patch: 

http://people.redhat.com/bkonrath/eclipse/eclipse-subclipse.spec.patch

Robert just rebuilt ganymed-ssh2 and javasvn to pick up the changes in rawhide
GCJ so it should be ready to go now. 

Robert, it would be nice if you could apply this patch and if you have time
update to the latest version (1.1.4). Thanks. 
Comment 27 Robert Marcano 2006-08-06 02:06:57 EDT
finally updated:

http://www.marcanoonline.com/downloads/fedora/package_submissions/subclipse/eclipse-subclipse-1.1.4-1.src.rpm
http://www.marcanoonline.com/downloads/fedora/package_submissions/subclipse/eclipse-subclipse.spec

I needed to do some tricky things because subclipse is now packaged as jar based
plugins; I unpackaged the core plugin, replaced internal libraries with symlinks
as previouly done, packaged core plugin classes as a jar, and added it to the
plugin MANIFEST; in order to generate native gcj libraries, use the javasn and
ganymed dependencies and remove external .class files.

I have noticed that fewer plugins are packages as directories, is
aot-compile-rpm able to compile jar files inside jar plugins?

Everything was build on mock, not tested on a real FC-6 yet. The 1.0.x version
is still available because version 1.1.x apparently is not compatible with
eclipse 3.1
Comment 28 Jason Tibbitts 2006-08-08 20:26:05 EDT
OK folks, this builds but installation is a bit odd:

GC Warning: Couldn't read /proc/stat
GC Warning: GC_get_nprocs() returned -1
GC Warning: Couldn't read /proc/stat
GC Warning: GC_get_nprocs() returned -1
dirname: missing operand
Try `dirname --help' for more information.
mkdir: missing operand
Try `mkdir --help' for more information.
GC Warning: Couldn't read /proc/stat
GC Warning: GC_get_nprocs() returned -1
/usr/bin/rebuild-gcj-db: line 17:  4325 Segmentation fault     
/usr/bin/gcj-dbtool -n $dbLocation 64
GC Warning: Couldn't read /proc/stat
GC Warning: GC_get_nprocs() returned -1
xargs: /usr/bin/gcj-dbtool: terminated by signal 11
GC Warning: Couldn't read /proc/stat
GC Warning: GC_get_nprocs() returned -1

Any idea what this is about?  It's an install in a mock chroot, so perhaps
there's some weirdness due to that, but I doubt it's reasonable for things to
segfault.

rpmlint has some complaints:

On the srpm:

W: eclipse-subclipse mixed-use-of-spaces-and-tabs
   Some lines are indented with tabs, some with spaces, and some of the
Requires: and BuildRequires: lines have both.

W: eclipse-subclipse patch-not-applied Patch10:
eclipse-subclipse-1.1.4-plugin-classpath.patch
  Not sure what's up here; perhaps a comment as to why this isn't applied would
help.

On the binary rpm:
W: eclipse-subclipse non-standard-group Text Editors/Integrated Development
Environments (IDE)
  I don't think there's any concensus as to what to do with groups at this
point; following Eclipse is probably best.

W: eclipse-subclipse invalid-license EPL
  Seems OK as that's what Eclipse uses.

W: eclipse-subclipse no-documentation
  Indeed, there's nothing marked as %doc.  Is there anything that should be so
marked?  There are license files as plain text and HTML changelog files and
such, which seems like they qualify.

W: eclipse-subclipse dangling-symlink
/usr/share/eclipse/plugins/org.tigris.subversion.subclipse.core_1.1.4/lib/javasvn.jar
/usr/share/java/javasvn.jar
  This is OK; it's a symlink to a dependency.

W: eclipse-subclipse symlink-should-be-relative
/usr/share/eclipse/plugins/org.tigris.subversion.subclipse.core_1.1.4/lib/javasvn.jar
/usr/share/java/javasvn.jar
  However, the link should be relative.

The symlink warnings are repeated for these files:
/usr/share/eclipse/plugins/org.tigris.subversion.subclipse.core_1.1.4/lib/ganymed.jar
/usr/share/eclipse/plugins/org.tigris.subversion.subclipse.core_1.1.4/lib/svnjavahl.jar

Comment 29 Robert Marcano 2006-08-22 00:00:28 EDT
Updated:

http://www.marcanoonline.com/downloads/fedora/package_submissions/subclipse/eclipse-subclipse-1.1.5-1.fc6.src.rpm
http://www.marcanoonline.com/downloads/fedora/package_submissions/subclipse/eclipse-subclipse.spec

> W: eclipse-subclipse patch-not-applied Patch10:
> eclipse-subclipse-1.1.4-plugin-classpath.patch
>   Not sure what's up here; perhaps a comment as to why this isn't applied would
> help.

comment added and it was changed to a %Source tag, this patch is applied at
install time, not at the "prep" stage

> W: eclipse-subclipse no-documentation
>  Indeed, there's nothing marked as %doc.  Is there anything that should be so
> marked?  There are license files as plain text and HTML changelog files and
> such, which seems like they qualify.

I added the svnClientAdapter related readme.txt and changelog.txt, subclipse
uses eclipse help system, and it is packages the same way that eclipse-cdt (no
documentation)

> W: eclipse-subclipse symlink-should-be-relative
>
/usr/share/eclipse/plugins/org.tigris.subversion.subclipse.core_1.1.4/lib/javasvn.jar
> /usr/share/java/javasvn.jar
>  However, the link should be relative.

All eclipse related RPMs package dependencies as absolute links, i just did the
same.
Comment 30 Jason Tibbitts 2006-08-24 00:46:43 EDT
Things look very good and I will work up a full review tomorrow, but I wanted to
comment on one thing:

> All eclipse related RPMs package dependencies as absolute links, i just did the
> same.

Given the state of many core packages, unless you can point to an Extras-style
review then pointing to what an existing package does is not generally a valid
argument.

Still, in this case I asked around and the issue of the
symlink-should-be-relative warning is not clear cut.  The main reason for the
warning is the fact that absolute symlinks get in the way of proper operation
with chroots (since the links will point to different files depending on whether
you've chrooted or not).  Symlinks can have this problem too if they contain
excessive ".." components, but hopefully that would be caught in a review.

So I tend to believe that it would be better to use relative symlinks, but it's
not really essential for a desktop application and in this case the rest of
eclipse is bound to have the same issue.  I won't block on it.
Comment 31 Jason Tibbitts 2006-08-25 00:50:34 EDT
I had to chase dependencies a bit to make sure that %libdir/gjc is properly
owned, but I'm pretty sure it is.  I wish there was a handy recursive dependency
chaser.

* source files match upstream.
   (No checksums possible; see above.  Comparison using diff shows the source to
be identical.)
* package meets naming and packaging guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is correct.
* license field matches the actual license.
* license is open source-compatible.  License text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate (as far as I can tell for Java)
* %clean is present.
* package builds in mock (development, x86_64).
* debuginfo package looks complete (no source, but there are debug symbols).
* rpmlint complaints are acceptable.
* final provides and requires are sane:
  eclipse-subclipse-1.1.5-1.fc6.x86_64.rpm
   org.tigris.subversion.subclipse.ui_1.1.5.jar.so()(64bit)
   subclipse-core.jar.so()(64bit)
   svnClientAdapter.jar.so()(64bit)
   eclipse-subclipse = 1.1.5-1.fc6
  =
   /usr/bin/rebuild-gcj-db
   eclipse-platform
   ganymed-ssh2 >= 209
   java-gcj-compat >= 1.0.33
   javasvn >= 1.1.0
   libgcj_bc.so.1()(64bit)
   librt.so.1()(64bit)
   libz.so.1()(64bit)
   subversion-javahl >= 1.3.1

  eclipse-subclipse-book-1.1.5-1.fc6.x86_64.rpm
   eclipse-subclipse-book = 1.1.5-1.fc6
  =
   eclipse-subclipse = 1.1.5-1.fc6

* %check is not present; no test suite upstream.
* no shared libraries are added to the regular linker search paths.
* package is not relocatable.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* scriptlets present (rebuild-gcj-db) are OK.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.  The subversion
book is provided as an eclipse plugin in a separate package.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no libtool .la droppings.

APPROVED

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