Bug 640205 - Review Request: visualvm - Lightweight profiler that integrates many command-line JDK tools
Summary: Review Request: visualvm - Lightweight profiler that integrates many command-...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Stanislav Ochotnicky
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-10-05 08:31 UTC by jiri vanek
Modified: 2011-04-18 12:12 UTC (History)
6 users (show)

Fixed In Version: visualvm-1.3.1-1.1.6.fc14
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-02-13 08:54:42 UTC
Type: ---
sochotni: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description jiri vanek 2010-10-05 08:31:38 UTC
SRPM URL: http://koji.fedoraproject.org/koji/getfile?taskID=2513496&name=visualvm-1.3-1.fc15.src.rpm

Description: VisualVM is a visual tool integrating several command-line JDK tools and lightweight profiling capabilities. Designed for both production and
development time use, it further enhances the capability of monitoring and
performance analysis for the Java SE platform. 

This tool have been extracted from openJDK, and is crucial for next release of openJDK for fedora.

Comment 1 jiri vanek 2010-10-05 08:45:33 UTC
Package Change Request
======================
Package Name: visualvm
New Branches: f14 f15
Owners: jvanek dbhole
InitialCC: jvanek dbhole

Comment 2 Jason Tibbitts 2010-10-05 13:37:24 UTC
This is a bit confusing.  You've submitted a change request, but the package does not exist so there's nothing to change.  It looks like this is a plain review ticket, but it hasn't been approved or even reviewed so you can't be submitting a request to create the git tree yet.

So what's actually going on here, and what do you need from the SCM admins?

Comment 3 Jason Tibbitts 2010-10-05 13:39:45 UTC
Oh, and just FYI, f15 hasn't branched yet; it's a few months too early to request f15 branches.

Comment 4 jiri vanek 2010-10-05 13:42:10 UTC
fedora‑cvs ? flag removed. Sorry for being to fast.

Comment 5 Andrew John Hughes 2010-10-05 13:45:07 UTC
The package needs approving to resolve https://bugzilla.redhat.com/show_bug.cgi?id=631360

Comment 6 Stanislav Ochotnicky 2010-10-05 18:35:08 UTC
I'll have a look at it

Comment 7 Stanislav Ochotnicky 2010-10-06 09:18:28 UTC
Before even starting official review...
visualvm.x86_64: W: invalid-license GPLv2 + Classpath Exception
visualvm.x86_64: W: non-etc-or-var-file-marked-as-conffile /usr/lib64/visualvm/etc/visualvm.clusters
visualvm.x86_64: W: conffile-without-noreplace-flag /usr/lib64/visualvm/etc/visualvm.clusters
visualvm.x86_64: W: non-etc-or-var-file-marked-as-conffile /usr/lib64/visualvm/etc/visualvm.conf
visualvm.x86_64: W: conffile-without-noreplace-flag /usr/lib64/visualvm/etc/visualvm.conf
visualvm.x86_64: W: no-documentation
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-modules-options-keymap.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-libs-osgi.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-openide-compat.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-modules-spi-actions.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-core-output2.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-modules-core-kit.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-modules-favorites.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-api-visual.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-libs-junit4.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-core-netigso.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-libs-felix.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-core-execution.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-jdesktop-layout.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-openide-options.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-openide-util-enumerations.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-libs-jsr223.xml_hidden
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-core-osgi.xml_hidden
visualvm.x86_64: W: hidden-file-or-dir /usr/lib64/visualvm/profiler/.lastModified
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/profiler/.lastModified
visualvm.x86_64: W: dangling-symlink /usr/lib64/visualvm/platform /usr/share/netbeans/platform12
visualvm.x86_64: W: no-manual-page-for-binary jvisualvm
visualvm.x86_64: W: non-standard-dir-in-usr jvm
visualvm-debuginfo.x86_64: W: invalid-license GPLv2 + Classpath Exception
visualvm-debuginfo.x86_64: E: debuginfo-without-sources
visualvm.src: W: spelling-error Summary(en_US) profiler -> profile, profiles, profiled
visualvm.src: W: invalid-license GPLv2 + Classpath Exception
3 packages and 0 specfiles checked; 19 errors, 13 warnings.

Need I comment on this? Please run rpmlint before putting packages for review. If there are good reasons for some of these things, explain...otherwise please fix them before I continue.

Comment 8 jiri vanek 2010-10-07 12:37:06 UTC
Be sure I'm runing rpmlint ;) Most of this is caused by upstream, And I can do nearly nothing with it. 

http://koji.fedoraproject.org/koji/getfile?taskID=2519784&name=visualvm-1.3-2.fc14.src.rpm

here is bugfix and my comments (going out of upstream):
visualvm.src: W: invalid-license GPLv2 + Classpath Exception
 -This is how upstream has chosen to License it, and it is a widely known license type.

visualvm.src: W: patch-not-applied Patch0: visualvm-debuginfo.patch
 -patch is applied, because of it's nature, in build. And IS applied

visualvm.x86_64: W: non-etc-or-var-file-marked-as-conffile /usr/lib64/visualvm/etc/visualvm.clusters
visualvm.x86_64: W: non-etc-or-var-file-marked-as-conffile /usr/lib64/visualvm/etc/visualvm.conf
 -Upstream maintains conf files within %{_libdir}/visualvm/etc/ .. thereis nothing I can change about it

visualvm.x86_64: W: no-documentation
 -Upstream does not provide any.

visualvm.x86_64: E: zero-length /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-modules-options-keymap.xml_hidden
...
visualvm.x86_64: E: zero-length /usr/lib64/visualvm/profiler/.lastModified
 -Upstream generates them and it is best not to delete them as upstream may rely on them for something.

visualvm.x86_64: W: hidden-file-or-dir /usr/lib64/visualvm/profiler/.lastModified
 -Added by upstream build.

visualvm.x86_64: W: dangling-symlink /usr/lib64/visualvm /platform/usr/share/netbeans/platform12
 -visualvm depends on netbeans and the above target will exist when netbeans is installed. (tested by installation)

visualvm.x86_64: W: no-manual-page-for-binary jvisualvm
 -Binary launches a gui.

visualvm.x86_64: W: non-standard-dir-in-usr jvm
 -Created by upstream -- not much I can change.

visualvm-debuginfo.x86_64: W: invalid-license GPLv2 + Classpath Exception
3 packages and 0 specfiles checked; 18 errors, 12 warnings.


visualvm.x86_64: W: conffile-without-noreplace-flag /usr/lib64/visualvm/etc/visualvm.clusters
visualvm.x86_64: W: non-etc-or-var-file-marked-as-conffile /usr/lib64/visualvm/etc/visualvm.conf
-FIXED

visualvm-debuginfo.x86_64: E: debuginfo-without-sources
-FIXED (by upstream and in spec)

Comment 9 jiri vanek 2010-10-07 12:39:06 UTC
 GPLv2 + Classpath Exception
is good license (http://fedoraproject.org/wiki/Licensing#SoftwareLicenses)

Comment 10 Stanislav Ochotnicky 2010-10-07 13:45:25 UTC
(In reply to comment #8)
> Be sure I'm runing rpmlint ;) Most of this is caused by upstream, And I can do
> nearly nothing with it. 

first priority: packaging guidelines
second priority: making upstream happy

> http://koji.fedoraproject.org/koji/getfile?taskID=2519784&name=visualvm-1.3-2.fc14.src.rpm

Please next time provide separate link to spec file so it can be accessed directly without digging it up from srpm

> here is bugfix and my comments (going out of upstream):
> visualvm.src: W: invalid-license GPLv2 + Classpath Exception
>  -This is how upstream has chosen to License it, and it is a widely known
> license type.

This license name is invalid. This has nothing to do with upstream. You are supposed to use "Short name" column from https://fedoraproject.org/wiki/Licensing#Good_Licenses for license tag.

> visualvm.src: W: patch-not-applied Patch0: visualvm-debuginfo.patch
>  -patch is applied, because of it's nature, in build. And IS applied

Add comment to the spec file explaining this.

> visualvm.x86_64: W: non-etc-or-var-file-marked-as-conffile
> /usr/lib64/visualvm/etc/visualvm.clusters
> visualvm.x86_64: W: non-etc-or-var-file-marked-as-conffile
> /usr/lib64/visualvm/etc/visualvm.conf
>  -Upstream maintains conf files within %{_libdir}/visualvm/etc/ .. thereis
> nothing I can change about it
> 
> visualvm.x86_64: W: no-documentation
>  -Upstream does not provide any.

How about COPYING file?

> visualvm.x86_64: E: zero-length
> /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-modules-options-keymap.xml_hidden
> ...
> visualvm.x86_64: E: zero-length /usr/lib64/visualvm/profiler/.lastModified
>  -Upstream generates them and it is best not to delete them as upstream may
> rely on them for something.
>
> visualvm.x86_64: W: hidden-file-or-dir
> /usr/lib64/visualvm/profiler/.lastModified
>  -Added by upstream build.

Are they really needed? "Upstream may rely on them for something" is not the answer. If you can't figure out if these files are really needed, ask upstream. If there is no other way, remove these empty files and re-create them in post (remove in postun). Though I very much doubt it would harm anything if they were missing.
 
> visualvm.x86_64: W: dangling-symlink /usr/lib64/visualvm
> /platform/usr/share/netbeans/platform12
>  -visualvm depends on netbeans and the above target will exist when netbeans is
> installed. (tested by installation)

No problem here

> visualvm.x86_64: W: no-manual-page-for-binary jvisualvm
>  -Binary launches a gui.

no problem

> visualvm.x86_64: W: non-standard-dir-in-usr jvm
>  -Created by upstream -- not much I can change.

Really? You are using configure instead of %configure macro and you are not setting sysconfdir. I am guessing that's why etc dir ends up in /usr

> visualvm-debuginfo.x86_64: W: invalid-license GPLv2 + Classpath Exception
> 3 packages and 0 specfiles checked; 18 errors, 12 warnings.
> 
> 
> visualvm.x86_64: W: conffile-without-noreplace-flag
> /usr/lib64/visualvm/etc/visualvm.clusters
> visualvm.x86_64: W: non-etc-or-var-file-marked-as-conffile
> /usr/lib64/visualvm/etc/visualvm.conf
> -FIXED
> 
> visualvm-debuginfo.x86_64: E: debuginfo-without-sources
> -FIXED (by upstream and in spec)

I'll start official review, but you'll have to fix those issues one way or the other.

Comment 11 Stanislav Ochotnicky 2010-10-07 14:35:21 UTC
(In reply to comment #10)
> > visualvm.x86_64: E: zero-length
> > /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-modules-options-keymap.xml_hidden
> > ...
> > visualvm.x86_64: E: zero-length /usr/lib64/visualvm/profiler/.lastModified
> >  -Upstream generates them and it is best not to delete them as upstream may
> > rely on them for something.
> >
> > visualvm.x86_64: W: hidden-file-or-dir
> > /usr/lib64/visualvm/profiler/.lastModified
> >  -Added by upstream build.
> 
> Are they really needed? "Upstream may rely on them for something" is not the
> answer. If you can't figure out if these files are really needed, ask upstream.
> If there is no other way, remove these empty files and re-create them in post
> (remove in postun). Though I very much doubt it would harm anything if they
> were missing.

.lastModified files should be created in %post (removed in %postun). See
http://bits.netbeans.org/dev/javadoc/org-netbeans-bootstrap/overview-summary.html#java.io.File-.lastModified

As for the other zero-length files. I don't know their function, but unless we find a reason for them to stay they should be removed

Comment 12 Stanislav Ochotnicky 2010-10-07 14:37:45 UTC
(In reply to comment #10)
> > visualvm.x86_64: W: non-standard-dir-in-usr jvm
> >  -Created by upstream -- not much I can change.
> 
> Really? You are using configure instead of %configure macro and you are not
> setting sysconfdir. I am guessing that's why etc dir ends up in /usr

Actually this is caused by passing --prefix=%{_prefix}/jvm/java-openjdk-jvisualvm to configure. But this still means it's nothing to do with upstream. You chose that directory

Comment 13 Tomas Hurka 2010-10-08 12:45:07 UTC
(In reply to comment #10)
> (In reply to comment #8)
> > visualvm.x86_64: E: zero-length
> > /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-modules-options-keymap.xml_hidden
> > ...
> >  -Upstream generates them and it is best not to delete them as upstream may
> > rely on them for something.
> 
> Are they really needed? 
Yes, the zero-length files in visualvm/config/Modules/* are needed.

Comment 14 jiri vanek 2010-10-08 13:49:55 UTC
(In reply to comment #10)
> (In reply to comment #8)
skip
> 
> > here is bugfix and my comments (going out of upstream):
> > visualvm.src: W: invalid-license GPLv2 + Classpath Exception
> >  -This is how upstream has chosen to License it, and it is a widely known
> > license type.
> 
> This license name is invalid. This has nothing to do with upstream. You are
> supposed to use "Short name" column from
> https://fedoraproject.org/wiki/Licensing#Good_Licenses for license tag.
> 

fixed

> > visualvm.src: W: patch-not-applied Patch0: visualvm-debuginfo.patch
> >  -patch is applied, because of it's nature, in build. And IS applied
> 
> Add comment to the spec file explaining this.

fixed 
> 
> > visualvm.x86_64: W: non-etc-or-var-file-marked-as-conffile
> > /usr/lib64/visualvm/etc/visualvm.clusters
> > visualvm.x86_64: W: non-etc-or-var-file-marked-as-conffile
> > /usr/lib64/visualvm/etc/visualvm.conf
> >  -Upstream maintains conf files within %{_libdir}/visualvm/etc/ .. thereis
> > nothing I can change about it
> > 
> > visualvm.x86_64: W: no-documentation
> >  -Upstream does not provide any.
> 

fixed 

> 
> > visualvm.x86_64: E: zero-length
> > /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-modules-options-keymap.xml_hidden
> > ...
> > visualvm.x86_64: E: zero-length /usr/lib64/visualvm/profiler/.lastModified
> >  -Upstream generates them and it is best not to delete them as upstream may
> > rely on them for something.
> >
> > visualvm.x86_64: W: hidden-file-or-dir
> > /usr/lib64/visualvm/profiler/.lastModified
> >  -Added by upstream build.
> 
> Are they really needed? "Upstream may rely on them for something" is not the
> answer. If you can't figure out if these files are really needed, ask upstream.
> If there is no other way, remove these empty files and re-create them in post
> (remove in postun). Though I very much doubt it would harm anything if they
> were missing.

fixed - upstream rely on them, but dont'really on wether there is first empty line in them.

> 
> > visualvm.x86_64: W: dangling-symlink /usr/lib64/visualvm
> > /platform/usr/share/netbeans/platform12
> >  -visualvm depends on netbeans and the above target will exist when netbeans is
> > installed. (tested by installation)
> 
> No problem here
> 
> > visualvm.x86_64: W: no-manual-page-for-binary jvisualvm
> >  -Binary launches a gui.
> 
> no problem
> 
> > visualvm.x86_64: W: non-standard-dir-in-usr jvm
> >  -Created by upstream -- not much I can change.
> 
> Really? You are using configure instead of %configure macro and you are not
> setting sysconfdir. I am guessing that's why etc dir ends up in /usr
> 

default prefix is /usr

> > visualvm-debuginfo.x86_64: W: invalid-license GPLv2 + Classpath Exception
> > 3 packages and 0 specfiles checked; 18 errors, 12 warnings.
> > 
> > 
> > visualvm.x86_64: W: conffile-without-noreplace-flag
> > /usr/lib64/visualvm/etc/visualvm.clusters
> > visualvm.x86_64: W: non-etc-or-var-file-marked-as-conffile
> > /usr/lib64/visualvm/etc/visualvm.conf
> > -FIXED
> > 
> > visualvm-debuginfo.x86_64: E: debuginfo-without-sources
> > -FIXED (by upstream and in spec)
> 
> I'll start official review, but you'll have to fix those issues one way or the
> other.

(In reply to comment #11)
> (In reply to comment #10)
> > > visualvm.x86_64: E: zero-length
> > > /usr/lib64/visualvm/visualvm/config/Modules/org-netbeans-modules-options-keymap.xml_hidden
> > > ...
> > > visualvm.x86_64: E: zero-length /usr/lib64/visualvm/profiler/.lastModified
> > >  -Upstream generates them and it is best not to delete them as upstream may
> > > rely on them for something.
> > >
> > > visualvm.x86_64: W: hidden-file-or-dir
> > > /usr/lib64/visualvm/profiler/.lastModified
> > >  -Added by upstream build.
> > 
> > Are they really needed? "Upstream may rely on them for something" is not the
> > answer. If you can't figure out if these files are really needed, ask upstream.
> > If there is no other way, remove these empty files and re-create them in post
> > (remove in postun). Though I very much doubt it would harm anything if they
> > were missing.
> 
> .lastModified files should be created in %post (removed in %postun). See
> http://bits.netbeans.org/dev/javadoc/org-netbeans-bootstrap/overview-summary.html#java.io.File-.lastModified

fixed


Current status:

visualvm.x86_64: W: non-etc-or-var-file-marked-as-conffile /usr/lib64/visualvm/etc/visualvm.clusters
visualvm.x86_64: W: non-etc-or-var-file-marked-as-conffile /usr/lib64/visualvm/etc/visualvm.conf
  -installed by upstream. Stil no clue how to fix :(

visualvm.x86_64: W: dangling-symlink /usr/lib64/visualvm/platform /usr/share/netbeans/platform12
visualvm.x86_64: W: no-manual-page-for-binary jvisualvm
visualvm.x86_64: W: non-standard-dir-in-usr jvm 
  - default prefix is /usr
2 packages and 0 specfiles checked; 0 errors, 5 warnings.

visualvm.src: W: spelling-error Summary(en_US) profiler -> profile, profiles, profiled
visualvm.src: W: patch-not-applied Patch0: visualvm-debuginfo.patch
1 packages and 0 specfiles checked; 0 errors, 2 warnings.
visualvm.spec: W: patch-not-applied Patch0: visualvm-debuginfo.patch
0 packages and 1 specfiles checked; 0 errors, 1 warnings.
^explained in specfile comment^

Comment 18 Stanislav Ochotnicky 2010-10-08 15:50:17 UTC
Package Review
==============

Key:
- = N/A
x = Check
! = Problem
? = Not evaluated

=== REQUIRED ITEMS ===
[!]  Rpmlint output:
visualvm.src: W: spelling-error Summary(en_US) profiler -> profile, profiles, profiled
OK

visualvm.src: W: patch-not-applied Patch0: visualvm-debuginfo.patch
explained 

visualvm.x86_64: W: non-etc-or-var-file-marked-as-conffile /usr/lib64/visualvm/etc/visualvm.clusters
visualvm.x86_64: W: non-etc-or-var-file-marked-as-conffile
/usr/lib64/visualvm/etc/visualvm.conf

so once again...Why didn't you use --sysconfdir switch to configure?

visualvm.x86_64: W: dangling-symlink /usr/lib64/visualvm/platform
/usr/share/netbeans/platform12

explained

visualvm.x86_64: W: no-manual-page-for-binary jvisualvm
visualvm.x86_64: W: non-standard-dir-in-usr jvm
I'll get to this further down


[x]  Package is named according to the Package Naming Guidelines[1].
[x]  Spec file name must match the base package name, in the format %{name}.spec.
[x]  Package meets the Packaging Guidelines[2].
[x]  Package successfully compiles and builds into binary rpms.
[x]  Buildroot is correct (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
It's actually
%(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
So while you're making other changes please change this as well

[x]  Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines[3,4].
[x]  License field in the package spec file matches the actual license.
License type: GPLv2 with exceptions
[x]  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 is included in %doc.
[-]  All independent sub-packages have license of their own
[x]  Spec file is legible and written in American English.
[x]  Sources used to build the package matches the upstream source, as provided in the spec URL.
MD5SUM this package    : 3dac8c562bc847d2782bdc192826271f
MD5SUM upstream package: 3dac8c562bc847d2782bdc192826271f

netbeans-profiler: aeca7cbd0f1dfb30858d80fa1e150820
visualvm_13: f1a28e24451982114be6590ceddeae12

[x]  All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines[5].
[x]  Package must own all directories that it creates.
[!]  Package requires other packages for directories it uses.
/usr/lib[64]/visualvm directory doesn't seem to be provided by any
other package and this one doesn't include it either

[!]  Package does not contain duplicates in %files.
files /usr/jvm/java-openjdk-jvisualvm/bin/jvisualvm
and /usr/bin/jvisualvm
are the same

plus file /usr/jvm/java-openjdk-jvisualvm/share/visualvm.desktop is in
wrong place (you can just delete it since you have desktop file in
right place as well)

[x]  Permissions on files are set properly.
[x]  Package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[x]  Package consistently uses macros.
[x]  Package contains code, or permissable content.
[x]  Fully versioned dependency in subpackages, if present.
[x]  Package contains a properly installed %{name}.desktop file if it is a GUI application.
[x]  Package does not own files or directories owned by other packages.
[!]  Package uses %global not %define
replace your %define-s with %global
[-]  If package uses tarball from VCS include comment how to re-create that tarball (svn export URL, git clone URL, ...)
[!]  If source tarball includes bundled jar/class files these need to
be removed prior to building

there is file nbbuild/sierra/build.jar in
netbeans-profiler-visualvm_release69.tar.gz

File includes *java file so it should be possible to re-create it
should it be necessary. Since the upstream tarball contains
binary file you'll have to unpack & clean & repack prior to building
visualvm (or create script to do it and include modified version in
SourceX:)

[x]  All filenames in rpm packages must be valid UTF-8.

=== Other suggestions ===
[-]  Package has BuildArch: noarch (if possible)
[x]  Latest version is packaged.
[x]  Reviewer should test that the package builds in mock.
Tested on: fedora-rawhide-x86_64


=== Issues ===
1. etc directory placement
2. ownership of /usr/lib[64]/visualvm directory
3. duplicate files in /usr/jvm (and existence of this directory)
4. define -> global macro
5. bundled build.jar inside netbeans tar.gz

Comment 19 Stanislav Ochotnicky 2010-10-08 15:55:10 UTC
Almost forgot:
6. You are not deleting .lastModified in postun
7. sed should be used in %prep, worst case %build section not %install
8. why are you using --prefix=%{_prefix}/jvm/java-openjdk-jvisualvm when you copy from that directory to /usr ?

Comment 21 Stanislav Ochotnicky 2010-11-01 15:00:38 UTC
The spec/srpm is mostly good now but the app itself doesn't work. It fails with:
$ /usr/bin/jvisualvm
/bin/bash: /platform*/lib/nbexec: No such file or directory

You will need to fix this before I continue the review. Please don't put things on review when you know they don't work. It doesn't look nice...

Apart from that the spec file looks pretty good.  but since the configure script is apparently ignoring --sysconfdir there is no reason to pass it there. You should make a comment when you are fixing paths that will state why you have to do it. Every non-standard thing should be explained. Then you have comments like this in file section:

#dir _libdir/visualvm/etc
#config(noreplace) _libdir/visualvm/etc/visualvm.conf
#config(noreplace) _libdir/visualvm/etc/visualvm.clusters

They are useless and just clutter the spec file. If I saw this in an old spec file..no problem. But there is no reason to have this in brand new specs. It's ugly and useless.

You are moving config files to /etc in install which is OK, but you should check if the application will actually use them. If not-> symlinks to original location.

Comment 22 jiri vanek 2010-11-02 07:44:13 UTC
(In reply to comment #21)

> You are moving config files to /etc in install which is OK, but you should
> check if the application will actually use them. If not-> symlinks to original
> location.


This is verified. Application is looking for them in /etc

Comment 23 jiri vanek 2010-11-16 08:04:34 UTC
http://jvanek.fedorapeople.org/visualvm-1.3.4.tar.gz
http://jvanek.fedorapeople.org/visualvm.spec

Added symlinks to correct locations to make it work.

Comment 24 jiri vanek 2010-11-19 10:31:02 UTC
http://jvanek.fedorapeople.org/visualvm-1.3.5.tar.gz
http://jvanek.fedorapeople.org/visualvm.spec

Damn. In 1.3.4 was missing slash in symlink causing non-run ability.

Comment 26 jiri vanek 2010-11-24 08:48:51 UTC
updated license.

Comment 28 Stanislav Ochotnicky 2010-11-24 13:50:03 UTC
Sooo...ta-da. The package is good enough now so that I can approve it without a problem. 

Two notes:
 * Add description why the license tag is the way it is (see [1])
 * In the meantime version 1.3.1 of visualvm was released. I don't expect big changes, so should be easy to update. Please do so before doing the final push/build.

[1] http://lists.fedoraproject.org/pipermail/legal/2010-November/001465.html

package is APPROVED.

Comment 29 jiri vanek 2010-11-24 14:34:29 UTC
New Package SCM Request
=======================
Package Name: visualvm
Short Description: Lightweight profiler that integrates many command-line JDK tools
Owners: jvanek dbhole
Branches: f13 f14 f15
InitialCC: jvanek dbhole

Comment 30 Andrew John Hughes 2010-11-24 16:42:22 UTC
Can I please have a list of what needs changing in the VisualVM harness?

To update to 1.3.1 will mean a new VisualVM harness release anyway, so may as well fix everything for a 1.1 release.

Comment 31 Stanislav Ochotnicky 2010-11-24 17:03:35 UTC
Paths where installation occurs are completely non-standard.
From quick look/memory refresh:
 * ignoring of --sysconfdir switch and installing config files to 
   %{_libdir}/visualvm/etc/ 
 * installing non-standard subdirectories into --prefix directory. I remember 
   there was "platform" directory and 2 others (can't remember names). These 
   should be placed into standard subdirectories. Either share or lib or whatever 
   is appropriate for that data.

That is probably all of it, but since there will be changes in the build process it will have to be verified.

Comment 32 Andrew John Hughes 2010-11-24 19:33:29 UTC
Thanks.  I'll try and take a look at these issues next week.

Comment 33 Jason Tibbitts 2010-11-24 20:30:51 UTC
It is way too early to request f15 branches.

Git done (by process-git-requests).

Comment 34 Alexander Kurtakov 2010-11-24 20:46:31 UTC
(In reply to comment #32)
> Thanks.  I'll try and take a look at these issues next week.

You are not supposed to touch review flag unless you're the reviewer. By setting it back to ? you have formally invalidated Stanislav's review and made it look like Jason has created git repo for unapproved package.

Comment 35 Andrew John Hughes 2010-11-24 21:55:46 UTC
I haven't touched any flags; just responded to the bug.

Besides, there is still work to be done before this can go into Fedora as Stanislav request a rebase on 1.3.1 which will mean a new upstream release.

Comment 36 Alexander Kurtakov 2010-11-24 22:04:09 UTC
(In reply to comment #35)
> I haven't touched any flags; just responded to the bug.
> 
Just look at the Flags: 
ahughes: 	fedora‑review 	?

Comment 37 Andrew John Hughes 2010-11-24 23:15:53 UTC
I didn't say it wasn't there.  I said I didn't set it, which I didn't.  I didn't even know such a flag existed.  It must be some automated result of posting on the bug.

Comment 38 Jason Tibbitts 2010-11-24 23:39:27 UTC
We do occasionally see some bizarre changes to tickets when people post to them.  Usually it's the component which changes (and generally to OxFFFF, the first component in the list) but I wouldn't be surprised if the flags occasionally changed randomly as well.

Stanislav, would you be so kind as to set it back to '+'?

Comment 39 Stanislav Ochotnicky 2010-11-25 07:43:46 UTC
(In reply to comment #38)
> Stanislav, would you be so kind as to set it back to '+'?

Done.

Comment 40 Fedora Update System 2010-11-25 12:09:37 UTC
visualvm-1.3-9.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/visualvm-1.3-9.fc14

Comment 41 Fedora Update System 2010-11-26 01:05:55 UTC
visualvm-1.3-9.fc14 has been pushed to the Fedora 14 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update visualvm'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/visualvm-1.3-9.fc14

Comment 42 Andrew John Hughes 2010-11-26 14:34:08 UTC
Jason, thanks for a more reasoned response.

Stanislav, should we create a new bug to track the update to 1.3.1?

Jiri, nothing should be being pushed yet!

Comment 43 jiri vanek 2010-11-26 14:43:56 UTC
Yes. When new visualvm package will be prepared for going out, then I will ask for new review.

Comment 44 Stanislav Ochotnicky 2010-11-26 15:28:01 UTC
I guess I should have known this is gonna happen...the package as it is can go into Fedora so as far as I am concerned I am done here. I probably should have waited with "+" until update to 1.3.1.

There is no "package review" for updates so no new review is going to happen. If the maintainer decides he wants to push reviewed version into F-14, that's up to him, he hasn't done anything against the rules/guidelines.

You can create a bug to track progress on update to 1.3.1, but it will have nothing to do with this review. I (reviewer) am there to verify package is sane before entering fedora repositories, reviewers can't watch updates for all reviewed packages (I'd be watching ~50 additional packages in past 9 months if that was the case).

Comment 45 Mat Booth 2010-12-16 17:26:48 UTC
Hi,

As I noted in the Bodhi update [1] the .desktop file is wrong, VisualVM cannot be launched from the Applications menu.

Just thought I'd mention it here, too.

[1] https://admin.fedoraproject.org/updates/visualvm-1.3-9.fc14

Comment 46 jiri vanek 2011-01-05 10:03:44 UTC
Version with several minor fixes (thanx Booth and others!) was pushed to test.
http://koji.fedoraproject.org/koji/taskinfo?taskID=2701892
https://admin.fedoraproject.org/updates/visualvm-1.3-10.fc14

I have no intensions to push it to stable, because 1.3.2 harness is on the way.

Comment 47 jiri vanek 2011-01-06 13:59:17 UTC
package updated to visualvm 1.3.1, harness 1.1 and profiler 6.9.1
https://admin.fedoraproject.org/updates/visualvm-1.3.1-1.fc14

Comment 48 Fedora Update System 2011-01-12 22:36:27 UTC
visualvm-1.3.1-3.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/visualvm-1.3.1-3.fc14

Comment 49 Fedora Update System 2011-01-18 10:08:07 UTC
visualvm-1.3.1-1.1.4.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/visualvm-1.3.1-1.1.4.fc14

Comment 50 Fedora Update System 2011-02-01 10:13:36 UTC
visualvm-1.3.1-1.1.5.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/visualvm-1.3.1-1.1.5.fc14

Comment 51 Fedora Update System 2011-02-02 17:07:50 UTC
visualvm-1.3.1-1.1.6.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/visualvm-1.3.1-1.1.6.fc14

Comment 52 Fedora Update System 2011-02-13 08:54:32 UTC
visualvm-1.3.1-1.1.6.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 53 Fedora Update System 2011-03-14 11:22:39 UTC
visualvm-1.3.2-1.2.7.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/visualvm-1.3.2-1.2.7.fc14

Comment 54 jiri vanek 2011-04-18 10:47:53 UTC
Package Change Request
======================
Package Name: visualvm
New Branches: el5 el6
Owners: jvanek dbhole


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