Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.

Bug 213594

Summary: Review Request: eclipse-phpeclipse
Product: [Fedora] Fedora Reporter: Brandon Holbrook <fedora>
Component: Package ReviewAssignee: Anthony Green <green>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: ben, green, rpm
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-12-21 01:23:04 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:    
Bug Blocks: 163779    
Attachments:
Description Flags
Build log from mock
none
Latest mock build results on fc6 none

Description Brandon Holbrook 2006-11-02 06:16:37 UTC
Spec URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse.spec
SRPM URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse-1.1.8-8.src.rpm

Description: This is a RESUBMISSION of eclipse-phpeclipse after the last review (Bug 203025) Stalled.

Comment 1 Brandon Holbrook 2006-11-02 06:18:28 UTC
*** Bug 203205 has been marked as a duplicate of this bug. ***

Comment 2 Brandon Holbrook 2006-11-02 06:20:24 UTC
I mistyped the original ticket number in comment 0: the original really was Bug
203205

Comment 3 Anthony Green 2006-11-02 11:30:19 UTC
Brandon: I don't think it's OK to add your own license file to %doc, even if it
is the appropriate one.  

I believe this rule is....
MUST: If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(s) for the
package must be included in %doc.

I tried this once with vkeybd and see where it got me...
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=189889


Comment 4 Brandon Holbrook 2006-11-02 15:06:43 UTC
Wow that's funny, because all of mine and others' (that I know of) PEAR/PECL
packages manually include a copy of the PHP license.

The tarball DOES contain an .html version of the license, but I thought an ASCII
version would be more portable so I manually included it.  If you like, I can
just stick the .html in %doc and remove the .txt file.

Comment 5 Anthony Green 2006-11-02 15:15:31 UTC
(In reply to comment #4) 
> The tarball DOES contain an .html version of the license, but I thought an ASCII
> version would be more portable so I manually included it.  If you like, I can
> just stick the .html in %doc and remove the .txt file.

I think that would be best.  I'll do a review of your next SRPM.

AG


Comment 7 Anthony Green 2006-11-09 23:59:03 UTC
(In reply to comment #6)
> Spec URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse.spec
> SRPM URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse-1.1.8-9.src.rpm

This fails to build in mock for me.  I don't know why.  I'll attach the build log.

Comment 8 Anthony Green 2006-11-10 00:00:30 UTC
Created attachment 140853 [details]
Build log from mock

Comment 9 Ben Konrath 2006-11-10 00:54:14 UTC
That build error is happening because the eclipse SDK packages have changed a
little since the review was submitted. You need to build with either: 

eclipse -noSplash  \
        -application org.eclipse.ant.core.antRunner \
        -DjavacFailOnError=true \
        ...

or:

java -cp $SDK/startup.jar \
     -Dosgi.sharedConfiguration.area=%{_libdir}/eclipse/configuration          
                \
     -Duser.home=$homedir                        \
      org.eclipse.core.launcher.Main             \
     -DjavacFailOnError=true \
      ... 

I like the first build method better because it will automatically pick up
anything we add to the binary launcher to help discover things like new
eclipseextension locations, config locations etc. But I've been having some
problems with GCJ using that build method - GCJ seems to use up all the memory
on my system and then build gets an OutOfMemory exception and die. So if the
first build method doesn't work, then try the second. I hope this helps and
don't be afraid to ask if you have any questions.

Comment 10 Brandon Holbrook 2006-11-28 06:25:50 UTC
Spec URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse.spec
SRPM URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse-1.1.8-10.src.rpm

Okay I spent a couple days tweaking the java command and now have something that
builds again.

Ben, I tried using `eclipse` to do my build, but every time it died with a
'Could not open Display' error.  With or without '-noSplash', I couldn't get it
to work.  Is there another trick to get eclipse to run command-line-only?

Comment 11 Brandon Holbrook 2006-11-28 06:47:38 UTC
Spec URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse.spec
SRPM URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse-1.1.8-11.src.rpm

I bumped the eclipse-platform requirement to 3.2 to reflect the current eclipse
version in fc6/devel

Comment 12 Ben Konrath 2006-11-28 21:43:19 UTC
(In reply to comment #10)
> Ben, I tried using `eclipse` to do my build, but every time it died with a
> 'Could not open Display' error.  With or without '-noSplash', I couldn't get it
> to work.  Is there another trick to get eclipse to run command-line-only?

No, I can't think of anything off hand. It should be ok to use what you have in
-11 but just set the osgi.sharedConfiguration.area property:

java -cp $SDK/startup.jar \
     -Dosgi.sharedConfiguration.area=%{_libdir}/eclipse/configuration \
     ....

Let me know if you have any other questions. Ben

Comment 13 Brandon Holbrook 2006-11-29 04:39:44 UTC
Spec URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse.spec
SRPM URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse-1.1.8-12.src.rpm

Okay I've added that configuration line, but have a question or two.  The line
as you provided it didn't work.  /usr/lib64/eclipse/configuration doesn't exist
on my system (or in /usr/lib/ on my i386 system), nor has yum ever heard of a
package providing said directory.  There IS, however, a
/usr/share/eclipse/configuration, which looks like it contains the proper
contents.  SO, I changed the line you gave to =%{_datadir}/eclipse/configuration
and everything started working beautifully.  Is there some other configuration
info that is supposed to exist in libdir that I don't have, or was your line
just a mistype?

Comment 14 Ben Konrath 2006-11-29 19:47:11 UTC
You need 
-Dosgi.sharedConfiguration.area=%{_libdir}/eclipse/configuration for rawhide
because we changed the packages to be multilib compatible. We are putting out an
update to fc6 with this change as well. I'll report back when the updated
packages have been pushed. Sorry for not mentioning this in the last post. 

Comment 15 Anthony Green 2006-12-16 16:01:16 UTC
Created attachment 143855 [details]
Latest mock build results on fc6

This is the latest mock build result for this package.	Is it supposed to build
for FC6?

Comment 16 Andrew Overholt 2006-12-18 16:15:25 UTC
The osgi.sharedConfiguration.area is incorrect.  It needs to be as Ben mentioned
in comment #14.

Comment 17 Brandon Holbrook 2006-12-19 02:55:21 UTC
Spec URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse.spec
SRPM URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse-1.1.8-13.src.rpm

Yeah sorry, when the new eclipse RPMs came out I built new packages for myself
but forgot to add them to this review.  Enjoy.

Comment 18 Anthony Green 2006-12-19 21:42:27 UTC
(In reply to comment #17)
> Spec URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse.spec
> SRPM URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse-1.1.8-13.src.rpm

Thanks.  

This is almost ready.  See lines starting with 'X' below.

* package meets 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.
* source files match upstream (extracted from upstream cvs so no md5sum available.)
* latest version is being packaged.
* BuildRequires are proper.
* package builds in mock.

X rpmlint on eclipse-phpeclipse-1.1.8-13.fc6.i386.rpm says:
W: eclipse-phpeclipse wrong-file-end-of-line-encoding
/usr/share/eclipse/features/net.sourceforge.phpeclipse_1.1.8/cpl-v10.html

Fix it with sed in the %prep section: 
%{__sed} -i 's/\r//' src/path/to/cpl-v10.html

rpmlint on eclipse-phpeclipse-1.1.8-13.fc6.src.rpm says:
W: eclipse-phpeclipse strange-permission make-phpeclipse-source-archive.sh 0755

I think we can ignore this.

* final provides and requires are sane:
eclipse-phpeclipse-1.1.8-13.fc6.i386.rpm
  core.jar.so  
  debug.jar.so  
  externaltools.jar.so  
  launch.jar.so  
  phpeclipse.jar.so  
  phphelp.jar.so  
  smartyui.jar.so  
  ui.jar.so  
  webbrowser.jar.so  
  webcore.jar.so  
  xmlcore.jar.so  
  xmlui.jar.so  
  eclipse-phpeclipse = 1.1.8-13.fc6
==
  /usr/bin/rebuild-gcj-db   
  eclipse-platform >= 1:3.2.1
  httpd  
  java-gcj-compat >= 1.0.33
  php  

* shared libraries are present, but no ldconfig required.
* package is not relocatable.
X does not own the directories it creates.
  %files contains...
%{_libdir}/gcj/%{name}/*
  but I think that should be...
%{_libdir}/gcj/%{name}
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* %clean is present.
* %check is not present
* scriptlets OK
* code, not content.
* documentation is integrated, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no libtool .la droppings.
* not a GUI app (no .desktop file required).
* not a web app.




Comment 19 Brandon Holbrook 2006-12-20 04:31:02 UTC
Spec URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse.spec
SRPM URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse-1.1.8-14.src.rpm

Both taken care of

Comment 20 Anthony Green 2006-12-20 12:50:30 UTC
(In reply to comment #19)
> Spec URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse.spec
> SRPM URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse-1.1.8-14.src.rpm
> 
> Both taken care of

Thanks, but why did you add a call to "bash" after the end-of-line conversion?


Comment 21 Brandon Holbrook 2006-12-20 22:20:29 UTC
Spec URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse.spec
SRPM URL: http://theholbrooks.org/RPMS/eclipse-phpeclipse-1.1.8-15.src.rpm

That's a feature, not a bug :)  Actually it was leftover debugging that I forgot
to take out.

Comment 22 Anthony Green 2006-12-20 22:26:17 UTC
This package is APPROVED.  Thanks!


Comment 23 Brandon Holbrook 2006-12-21 01:23:04 UTC
Imported, FC6 branch requested, and build requested (job 24299)