Bug 191017

Summary: Review Request: eclipse-subclipse
Product: [Fedora] Fedora Reporter: Robert Marcano <robert>
Component: Package ReviewAssignee: Jason Tibbitts <j>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: ben, ifoox, overholt
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2006-08-27 01:56:52 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 191014, 191015    
Bug Blocks: 163779    
Attachments:
Description Flags
updated spec file
none
log from failed build none

Description Robert Marcano 2006-05-08 03:46:39 UTC
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 18:34:08 UTC
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 20:28:29 UTC
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 20:38:18 UTC
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> 1.0.1-3
- Rename package to eclispe-subclipse.
- Use copy-platform script for now.

Comment 5 Ben Konrath 2006-05-29 22:30:28 UTC
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 17:45:19 UTC
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-05 03:33:44 UTC
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 04:34:32 UTC
We may have lost some useful comments here. It would be great if people could
re-post anything still relevant. Thanks.

Comment 10 Robert Marcano 2006-06-26 03:23:20 UTC
Updated:

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

* Sun Jun 25 2006 Robert Marcano <robert> 1.0.3-1
- Update to 1.0.3
- Dependency name changed to ganymed-ssh2

Comment 11 Jason Tibbitts 2006-06-26 20:28:37 UTC
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 21:30:55 UTC
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 21:58:00 UTC
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 12:22:59 UTC
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-03 03:08:24 UTC
Removing bug #191016 from the dependecies list

Comment 17 Jason Tibbitts 2006-07-03 19:39:28 UTC
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 19:40:25 UTC
Created attachment 131887 [details]
log from failed build

Comment 19 Andrew Overholt 2006-07-03 19:50:15 UTC
(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 14:11:22 UTC
(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 16:32:23 UTC
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 17:07:18 UTC
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 20:43:59 UTC
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 21:39:43 UTC
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 21:48:35 UTC
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 19:48:34 UTC
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 06:06:57 UTC
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-09 00:26:05 UTC
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 04:00:28 UTC
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 04:46:43 UTC
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 04:50:34 UTC
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