Bug 905255 - Review Request: open-vm-tools - Open Virtual Machine Tools
Summary: Review Request: open-vm-tools - Open Virtual Machine Tools
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Simone Caronni
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
: 294321 789965 (view as bug list)
Depends On: 950748 1119255
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-01-29 00:41 UTC by Sankar Tanguturi
Modified: 2014-07-14 11:35 UTC (History)
13 users (show)

See Also:
(edit)
Clone Of:
(edit)
Last Closed: 2013-05-06 04:24:07 UTC
negativo17: fedora-review+


Attachments (Terms of Use)
Review fixes for the first part (6.54 KB, patch)
2013-04-09 17:03 UTC, Simone Caronni
no flags Details | Diff
Review fixes for the second part (3.07 KB, patch)
2013-04-09 20:50 UTC, Simone Caronni
no flags Details | Diff
Modified open-vm-tools.init (2.60 KB, patch)
2013-04-11 00:01 UTC, Ravindra Kumar
no flags Details | Diff

Description Sankar Tanguturi 2013-01-29 00:41:28 UTC
Spec URL: https://www.dropbox.com/sh/9880qa24fv1ofyb/9FOYpb0gsv/v1/open-vm-tools.spec
SRPM URL: https://www.dropbox.com/sh/9880qa24fv1ofyb/LIcMMucRRV/v1/open-vm-tools-9.2.2-1.fc16.src.rpm
Description: The open-vm-tools project is an open source implementation
of VMware Tools. They provide a suite of open source virtualization
utilities and drivers to improve the functionality and user experience
of VMware virtual machines. This package contains only the user-space
programs and libraries of open-vm-tools.
Fedora Account System Username: stanguturi

Check redhat bug 883614 for more details. We are planning to upstream 'open-vm-tools'.

I have never worked on packaging stuff. I got the basic spec work done. Looking forward for review comments. I will promptly update the spec with the review comments.

Comment 1 Mohamed El Morabity 2013-01-29 09:15:05 UTC
Hello,

(In reply to comment #0)
> Check redhat bug 883614 for more details. We are planning to upstream
> 'open-vm-tools'.
"Access denied" :(


Just an informal review for the moment. I tested your package on Fedora 18. I see your SRPM package is labelled "f16", be careful this version won't be maintained anymore in a few days.

1) The following dependencies are missing from the BuildRequires:
   - libXtst-devel
   Without them, I just can't build your package, the configure step fails.
   Some others are detected as unavailable during configure, although it doesn't block the build:
   - fuse-devel (probably to enable sharing support)
   - libSM-devel
   - libICE-devel
   I suggest you to build your packages using mock, to be sure not to forget any BR:
       http://fedoraproject.org/wiki/Projects/Mock

2) I have issues with procps which can't be found, although procps-ng-devel is installed:
   configure: error: libproc not found. Please configure without procps (using --without-procps) or install procps - http://procps.sourceforge.net
   It looks like libproc.so is detected by default by configure, whereas Fedora provides libprocps.so. To fix this issue, I had to:
   - add procps-ng-devel as BuildRequires
   - override the default procps library name detected using "export CUSTOM_PROCPS_NAME=procps" before %configure

3) I don't thing glib is really required by openvm-tools. I think you mean glib2 (according to configure.ac, only glib2 is needed). As a result, replace glib by glib2 in BuildRequires.

4) gtk2 is probably not the BuildRequires you want to use. You want gtk2-devel instead. gtk2-devel contains the headers and .so files needed to develop with GTK+2, and required by openvm-tools.

5) BuildRoot tag, BuildRoot cleaning and %clean sections are deprecated:
      http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag
      http://fedoraproject.org/wiki/Packaging:Guidelines#.25clean
   Please drop all these things.
   %defattr is useless too in your package:
      http://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions
   Please drop it from the %files section.

6) SysV init scripts are deprecated in favor of systemd unit files:
      http://fedoraproject.org/wiki/Packaging:Systemd
   You can't provide anymore such service file, you must convert it to Systemd.

7) Although all the fixes made above, the package build fails with the following error:

fileLogger.c: In function 'FileLoggerLog':
fileLogger.c:257:4: error: 'g_static_rw_lock_reader_lock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:210): Use 'g_rw_lock_reader_lock' instead [-Werror=deprecated-declarations]
fileLogger.c:268:7: error: 'g_static_rw_lock_reader_unlock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:216): Use 'g_rw_lock_reader_unlock' instead [-Werror=deprecated-declarations]
fileLogger.c:269:7: error: 'g_static_rw_lock_writer_lock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:219): Use 'g_rw_lock_writer_lock' instead [-Werror=deprecated-declarations]
fileLogger.c:273:7: error: 'g_static_rw_lock_writer_unlock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:225): Use 'g_rw_lock_writer_unlock' instead [-Werror=deprecated-declarations]
fileLogger.c:274:7: error: 'g_static_rw_lock_reader_lock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:210): Use 'g_rw_lock_reader_lock' instead [-Werror=deprecated-declarations]
fileLogger.c:291:13: error: 'g_static_rw_lock_reader_unlock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:216): Use 'g_rw_lock_reader_unlock' instead [-Werror=deprecated-declarations]
fileLogger.c:292:13: error: 'g_static_rw_lock_writer_lock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:219): Use 'g_rw_lock_writer_lock' instead [-Werror=deprecated-declarations]
fileLogger.c:298:13: error: 'g_static_rw_lock_writer_unlock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:225): Use 'g_rw_lock_writer_unlock' instead [-Werror=deprecated-declarations]
fileLogger.c:299:13: error: 'g_static_rw_lock_reader_lock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:210): Use 'g_rw_lock_reader_lock' instead [-Werror=deprecated-declarations]
fileLogger.c:309:4: error: 'g_static_rw_lock_reader_unlock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:216): Use 'g_rw_lock_reader_unlock' instead [-Werror=deprecated-declarations]
fileLogger.c: In function 'FileLoggerDestroy':
fileLogger.c:331:4: error: 'g_static_rw_lock_free' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:228): Use 'g_rw_lock_free' instead [-Werror=deprecated-declarations]
fileLogger.c: In function 'GlibUtils_CreateFileLogger':
fileLogger.c:378:4: error: 'g_static_rw_lock_init' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:207): Use 'g_rw_lock_init' instead [-Werror=deprecated-declarations]

   Could you check it with your devteam?

8) Why do you disable debug package generation(see %global debug_package %{nil}). This is something unacceptable for the Fedora Project, unless you really have a good reason to do it. Could you explain such a choice?

Comment 2 Mohamed El Morabity 2013-01-29 09:21:28 UTC
> 3) I don't thing glib is really required by openvm-tools. I think you mean
> glib2 (according to configure.ac, only glib2 is needed). As a result,
> replace glib by glib2 in BuildRequires.
Same issue than for gtk2: glib2-devel would be more appropriate than glib2 as BuildRequires.

Comment 3 Sankar Tanguturi 2013-01-31 01:58:13 UTC
Thanks for the review comments. I will address the review comments and update the spec files.

Comment 4 Mark Mikofski 2013-04-02 04:07:44 UTC
I had the same 2 issues as Mohamed 2 and 7, although I had to make a symlink to libproc.so --> libprocps.so to get past 2, since export CUSTOM_PROCPS_NAME=procps didn't work. Both of these are bugs that should be addressed upstream by open-vm-tools, if it is even being maintained anymore; there hasn't been a new release in a while.

here's my make traceback:
fileLogger.c: In function 'FileLoggerLog':
fileLogger.c:257:4: error: 'g_static_rw_lock_reader_lock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:210): Use 'g_rw_lock_reader_lock' instead [-Werror=deprecated-declarations]
fileLogger.c:268:7: error: 'g_static_rw_lock_reader_unlock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:216): Use 'g_rw_lock_reader_unlock' instead [-Werror=deprecated-declarations]
fileLogger.c:269:7: error: 'g_static_rw_lock_writer_lock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:219): Use 'g_rw_lock_writer_lock' instead [-Werror=deprecated-declarations]
fileLogger.c:273:7: error: 'g_static_rw_lock_writer_unlock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:225): Use 'g_rw_lock_writer_unlock' instead [-Werror=deprecated-declarations]
fileLogger.c:274:7: error: 'g_static_rw_lock_reader_lock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:210): Use 'g_rw_lock_reader_lock' instead [-Werror=deprecated-declarations]
fileLogger.c:291:13: error: 'g_static_rw_lock_reader_unlock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:216): Use 'g_rw_lock_reader_unlock' instead [-Werror=deprecated-declarations]
fileLogger.c:292:13: error: 'g_static_rw_lock_writer_lock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:219): Use 'g_rw_lock_writer_lock' instead [-Werror=deprecated-declarations]
fileLogger.c:298:13: error: 'g_static_rw_lock_writer_unlock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:225): Use 'g_rw_lock_writer_unlock' instead [-Werror=deprecated-declarations]
fileLogger.c:299:13: error: 'g_static_rw_lock_reader_lock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:210): Use 'g_rw_lock_reader_lock' instead [-Werror=deprecated-declarations]
fileLogger.c:309:4: error: 'g_static_rw_lock_reader_unlock' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:216): Use 'g_rw_lock_reader_unlock' instead [-Werror=deprecated-declarations]
fileLogger.c: In function 'FileLoggerDestroy':
fileLogger.c:331:4: error: 'g_static_rw_lock_free' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:228): Use 'g_rw_lock_free' instead [-Werror=deprecated-declarations]
fileLogger.c: In function 'GlibUtils_CreateFileLogger':
fileLogger.c:378:4: error: 'g_static_rw_lock_init' is deprecated (declared at /usr/include/glib-2.0/glib/deprecated/gthread.h:207): Use 'g_rw_lock_init' instead [-Werror=deprecated-declarations]
cc1: all warnings being treated as errors
make[2]: *** [libGlibUtils_la-fileLogger.lo] Error 1
make[2]: Leaving directory `/home/whoami/Downloads/open-vm-tools-9.2.2-893683/lib/glibUtils'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/whoami/Downloads/open-vm-tools-9.2.2-893683/lib'
make: *** [all-recursive] Error 1

Comment 5 Mark Mikofski 2013-04-02 04:52:09 UTC
use:

    sudo ./configure CFLAGS=-Wno-deprecated-declarations

to remove deprecation warnings treated as errors

isntalls fine as long as you also link to libprocps.so

Of course this is a short term fix. I tried looking into replacing the depracated functions

    g_static_rw_lock_reader_lock --> g_rw_lock_reader_lock
    g_static_rw_lock_reader_unlock --> g_rw_lock_reader_unlock
    g_static_rw_lock_writer_lock --> g_rw_lock_writer_lock
    g_static_rw_lock_writer_unlock --> g_rw_lock_writer_unlock
    g_static_rw_lock_free --> g_rw_lock_free
    g_static_rw_lock_init --> g_rw_lock_init

but the function signatures are not the same, so it would take an investment

Comment 6 Ravindra Kumar 2013-04-09 03:57:25 UTC
I took up Sankar's original submission and made following changes:

 - Modified SPEC to follow the conventions and guidelines
 - Addressed review comments from Mohamed El Morabity
 - Added systemd script
 - Verified and built the RPMS for Fedora 18
 - Fixed rpmlint warnings
 - Split the UX components in a separate package for desktops
 - Split the help files in a separate package for help
 - Split the guestlib headers in a separate devel package

Following are the updated SPEC file and SRPM:

Spec URL: https://www.dropbox.com/s/96qpmdrl3fzsw80/open-vm-tools.spec
SRPM URL: https://www.dropbox.com/s/2ndmgtgodbra59t/open-vm-tools-9.2.2-1.fc18.src.rpm

Please take another look and let me know if I need to make any other changes.

Comment 7 Ravindra Kumar 2013-04-09 05:04:40 UTC
My Fedora Account System Username: ravindrakumar

Comment 8 Ravindra Kumar 2013-04-09 05:25:28 UTC
This is my first package submission to Fedora. Therefore, I need an sponsor, marking FE-NEEDSPONSOR.

Comment 9 Simone Caronni 2013-04-09 15:54:01 UTC
I'm taking this for review as I'm actually a user of Fedora on VMWare 5.x.

I'm not a sponsor and I'm not sure I should request it as my time to work on Fedora it's not always consistent. Despite this, the review can proceed :)

I have a few notes on things to fix in the spec file, a message will follow in a while.

Comment 10 Ravindra Kumar 2013-04-09 16:42:58 UTC
Thanks Simone. I hope you are ok with doing review under this bug. In case you want me to file a new bug and mark this one as duplicate, let me know.

FYI, I would like to push this package in Fedora 19. So, I would really appreciate your review keeping Fedora 19 deadlines.

Comment 11 Simone Caronni 2013-04-09 17:02:01 UTC
Hello, no problem for the Fedora 19 deadlines, I think we can make it and we can also add the package to Fedora 17/18 and EPEL 5/6. I think it would be very beneficial for all the virtualisation needs. And I'm offering myself as co-mantainer as now currently I use official VMWare packages for all the current RHEL systems (5, 6) and no tools at all for the Fedora systems.

In the next post there are "some" things to fix; I'm attaching also a patch to your spec file as this is your first package and the list of changes is quite big.

Please understand the patch contents and then we will proceed with the other items remaining (systemd, spec file differences between EPEL / Fedora, etc.).

Comment 12 Simone Caronni 2013-04-09 17:02:27 UTC
1) Please use tabs or spaces consistently in the SPEC file; use tabs all over or spaces.

For example, all the "Group" lines use spaces and not tabs like the rest of the file.

2) Please reduce the number of variables at the beginning of the SPEC file, for example there's no reason for "toolspkgname" to exist, you could already use "%{name}" where required.

3) Please add a %doc directive to the base package, there are at least the files:

%doc AUTHORS ChangeLog COPYING INSTALL NEWS README

that can be added to the release. According to the packaging guidelines, you should have at least the license file installed in any combination of the packages/subpackages, so that is required at least in the base package.

4) File ownership...

The base package should own the %{_sysconfdir}/vmware-tools/ directory entirely and not only the files therein.

The same goes for the directory %{_libdir}/%{name}/; is not currently owned by any package.

There's a floating alone "%doc" line at the end of the file before the %changelog section, probably is there by mistake.

5) The Group "Application/Documentation" is not valid, please use "Documentation".

6) There's a check command that can be performed in the build, so please add a section before %post:

%check
make check

7) During a review, the %changelog should be upgraded and updated as you did for the comment part but also the revision number should change.

8) The install command should preserve timestamps (-p) and the systemd service files need not to be executable.

9) There's currently a subpackage defined as "help" but nobody is creating the specified folder listed in the files section and nobody is installing any document in it, so please:

- Add a Build requirement for Doxygen.
- Build the docs under the "help" directory, there's a Makefile. According to the configure script, all the documents should be built if doxygen is installed.
- If they are very large, you can add them into a doc subpackage as it is now; but only by marking them in the %doc section.
- If they are not, add them to the appropriate package as %doc lines. It appears they are quite big (~130kb); so a separate package is fine.

10) Please sort BuildRequires for easier reading.

11) The following directories should be owned by the base package, not only the files contained therein:

%{_libdir}/%{name}/
%{_libdir}/%{name}/plugins
%{_libdir}/%{name}/plugins/common/
%{_libdir}/%{name}/plugins/vmsvc/

This one should be owned by the desktop subpackage:

%{_libdir}/%{name}/plugins/vmusr/

12) The static archives (*.la) and unversioned shared objects (*.so) should be included in the devel subpackage. I think the static archives could be removed entirely.

13) The directory /sbin/ should be excluded entirely, not just its contents.

14) It's good practice that the description of packages/subpackages stay the closest to 80 columns (no more) after macros have been expanded.

Comment 13 Simone Caronni 2013-04-09 17:03:39 UTC
Created attachment 733289 [details]
Review fixes for the first part

Comment 14 Simone Caronni 2013-04-09 17:08:54 UTC
Please make sure to understand the patch contents and apply as is or integrate with your amendments.

I also made a mistake in the patch, the changelog version should be 9.2 not 1.0.

Please update the changelog and bump the release.

Comment 15 Ravindra Kumar 2013-04-09 19:05:45 UTC
Simone, thanks for the review and providing a fixes patch. I have incorporated your patch as is and made a few corrections in the files section for base and devel packages. Please find the updated files below.

SPEC File URL: https://www.dropbox.com/s/bfv10xe5hnxsvor/open-vm-tools.spec
SRPM URL: https://www.dropbox.com/s/nyga3zs6xilazhl/open-vm-tools-9.2.2-3.fc18.src.rpm

And, I can swear I did not mix tabs and spaces :-)

I have defined tab space=8 in my vimrc and I think you would have defined it as 4, because your alignment in patch is not working in my VIM until I set tab space to 4. Please let me know if space is preferred over tabs in general. I personally prefer space because of tab weirdness, but I'd like to follow Fedora convention for this submission.

I think it would be great if we can get this package in FC 18 as well, please let me know how to make that happen?

Looking forward for the next set of comments!

Comment 16 Simone Caronni 2013-04-09 19:28:51 UTC
(In reply to comment #15)
> And, I can swear I did not mix tabs and spaces :-)
> 
> I have defined tab space=8 in my vimrc and I think you would have defined it
> as 4, because your alignment in patch is not working in my VIM until I set
> tab space to 4. Please let me know if space is preferred over tabs in
> general. I personally prefer space because of tab weirdness, but I'd like to
> follow Fedora convention for this submission.

I have downloaded the file again and it contains tabs again, maybe it's a Dropbox "feature"? :D

I prefer spaces as well, and there's no general guidelines for this, only to be consistent on it all over the spec file.

> I think it would be great if we can get this package in FC 18 as well,
> please let me know how to make that happen?

We can make it happen (and I'm very interested in) for both Fedora and EPEL by making the spec file compatible with both distributions / releases. This involves a few things:

- Systemd specific BuildRequires/Requires/%post for Fedora 17/18
- Systemd specific BuildRequires/Requires/%post for Fedora 19 and RHEL/CentOS 7+
- SysV init specific BuildRequires/Requires/%post for RHEL/CentOS 5/6
- SysV init script for RHEL/CentOS 5/6

Your spec file already contains a bit of everything, in particular, the systemd policies are for Fedora 17/18, the Group directive is for RHEL/CentOS 5 only.

Next set of comments in a few minutes.

Comment 17 Simone Caronni 2013-04-09 20:49:23 UTC
1) Service enablement

According to the Fedora policies no service should be started upon installation for the exception of those listed in this page:

https://fedoraproject.org/wiki/Starting_services_by_default

I agree that the guestd daemon should be started upon installation as this is installed only in VMWare guests.
After the review I will file a ticket to FESCO as required, to see if we can enable it:

https://fedorahosted.org/fesco/

So for now _all_ services should be installed but disabled by default.

2) Systemd conditionals

To enable Systemd for Fedora 17 and 18+, we need to apply the packaging policies as defined here:

http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd

As you see, there are differences between those 2 blocks (18+ and 17) so an "%if" between distributions should be applied
When f17 will be EOL, the block relevant to it can be removed.

3) RHEL/CentOS 5 building

RHEL/CentOS 5 has rpm 4.5 and needs some more settings for packages. A BuildRoot, and a %clean section.

To generate template spec files for the various distributions, use the "rpmdev-newspec" command. For example, the following 3 commands:

rpmdev-newspec -m -r 4.5 -o 45.spec
rpmdev-newspec -m -r 4.8 -o 48.spec
rpmdev-newspec -m -o 410.spec

4) SysV init script conditionals

SysV init scripts for RHEL/CentOS systems is defined in these policies:

https://fedoraproject.org/wiki/Packaging:SysVInitScript

Again we need to "%if" the distribution from Fedora.

You can create a SysV init script template with the "rpmdev-newinit open-vm-tools-guestd" command from the "rpmdevtools" package. Please create it, modify appropriately for RHEL and add it to the source package as "open-vm-tools-guestd.init"; it's the commented line with "Source2".

All the information you need is in the above link.

The init script should be enabled by default on runlevels 2-5 when "chkconfig --enable" is run; as the tools can be handy also in single user mode.

will generate rpm spec file templates for RHEL 5 (rpm 4.5), RHEL 6 (rpm 4.8) and Fedora 18+. You can tie the version to the generation, for example Fedora 19 uses rpm 4.11 (no changes to the template).

5) open-vm-tools-desktop

The file "/etc/xdg/autostart/vmware-user.desktop" contains a note about KDE, can you check that this is still valid?

6) open-vm-tools-guestd.service

Please see the following URL:

http://fedoraproject.org/wiki/Packaging:Systemd

- The Documentation directive is missing
- The lines "RestartSec=5" and "TimeoutStopSec=2" are probably not needed, as the process is contained in a cgroup
- Daemons should not be forking if possible, they should "stay" in the foreground as systemd takes care of that. Can the "Type=forking" line be removed?
- Probably you can make the description like "Service for virtual machines hosted on VMware" and remove the package name from it, but this is not required.

Comment 18 Simone Caronni 2013-04-09 20:49:47 UTC
Attached is a patch to your spec file with the changes for points 1-4.
As before, I have mixed tabs/spaces, so they do not match with yours, please apply / update as you see fit.

Comment 19 Simone Caronni 2013-04-09 20:50:48 UTC
Created attachment 733357 [details]
Review fixes for the second part

Comment 20 Simone Caronni 2013-04-09 20:57:01 UTC
Sorry, I made a mistake (damn touchpad). Just to make it clearer:

(In reply to comment #17)
> rpmdev-newspec -m -r 4.5 -o 45.spec
> rpmdev-newspec -m -r 4.8 -o 48.spec
> rpmdev-newspec -m -o 410.spec

> will generate rpm spec file templates for RHEL 5 (rpm 4.5), RHEL 6 (rpm 4.8)
> and Fedora 18+. You can tie the version to the generation, for example
> Fedora 19 uses rpm 4.11 (no changes to the template).

The second sentence at the end of point 4 was supposed to go at the end of point 3.


> The init script should be enabled by default on runlevels 2-5 when "chkconfig > --enable" is run; as the tools can be handy also in single user mode.

Well, runlevel 2 is multi user with no network, not single user mode.

Regards,
--Simone

Comment 21 Ravindra Kumar 2013-04-09 23:41:40 UTC
(In reply to comment #17)
> I agree that the guestd daemon should be started upon installation as this
> is installed only in VMWare guests.

Yes.

> After the review I will file a ticket to FESCO as required, to see if we can
> enable it:
> 
> https://fedorahosted.org/fesco/
> 
> So for now _all_ services should be installed but disabled by default.

Sure. Could you please CC me on the ticket?

> 2) Systemd conditionals
> 
> To enable Systemd for Fedora 17 and 18+, we need to apply the packaging
> policies as defined here:
> 
> http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd
> 
> As you see, there are differences between those 2 blocks (18+ and 17) so an
> "%if" between distributions should be applied
> When f17 will be EOL, the block relevant to it can be removed.
> 
> 3) RHEL/CentOS 5 building
> 
> RHEL/CentOS 5 has rpm 4.5 and needs some more settings for packages.
...
> 
> 4) SysV init script conditionals
> 
> SysV init scripts for RHEL/CentOS systems is defined in these policies:
> 
> https://fedoraproject.org/wiki/Packaging:SysVInitScript
> 
> Again we need to "%if" the distribution from Fedora.

Thanks for making all the changes and providing the patch. Given all these complexities and testing effort, I'm inclined to focus on Fedora 18+ and RHEL 7+, because those are almost the same from systemd perspective. That will help me keep the SPEC file simple and testing is easy for me. Hope you would agree with me?

> 5) open-vm-tools-desktop
> 
> The file "/etc/xdg/autostart/vmware-user.desktop" contains a note about KDE,
> can you check that this is still valid?

I looked at the bug https://bugs.kde.org/show_bug.cgi?id=190522 and it seems to have been fixed in a way that it is not reproducible. However, I will not be able to fix/test the note right now, because this file is coming from open-vm-tools source and we will need to fix the source code as sourceforge. I think we will need to raise a ticket for open-vm-tools code.

> 6) open-vm-tools-guestd.service
> 
> Please see the following URL:
> 
> http://fedoraproject.org/wiki/Packaging:Systemd
> 
> - The Documentation directive is missing

I don't see any man pages generated from open-vm-tools build, so I have just added a link to http://open-vm-tools.sourceforge.net/about.php.

> - The lines "RestartSec=5" and "TimeoutStopSec=2" are probably not needed,
> as the process is contained in a cgroup

Removed RestartSec. TimeoutStopSec default is 90s which is way too long from service stop and guest shutdown perspectives. Please note that tools service does not handle SIGTERM nicely, so systemd ends up timing out and issues SIGKILL ultimately to kill this service. I believe this also needs to be fixed in open-vm-tools source code.

> - Daemons should not be forking if possible, they should "stay" in the
> foreground as systemd takes care of that. Can the "Type=forking" line be
> removed?

I had started with "simple" service and had some issues. So, I set it to forking, I will try again with "simple".

> - Probably you can make the description like "Service for virtual machines
> hosted on VMware" and remove the package name from it, but this is not
> required.

Changed. I will share the updated the SPEC file and SRPM in a few minutes.

Comment 22 Ravindra Kumar 2013-04-10 00:25:10 UTC
(In reply to comment #16)
> I have downloaded the file again and it contains tabs again, maybe it's a
> Dropbox "feature"? :D
> 
> I prefer spaces as well, and there's no general guidelines for this, only to
> be consistent on it all over the spec file.

I did not mean I did not use tabs, I meant I did not mix tabs and spaces :-)

Please find the updated SPEC file and SRPM below. I have replaced tabs with spaces now.

SPEC File URL: https://www.dropbox.com/s/279c1xbqv49ulro/open-vm-tools.spec
SRPM URL: https://www.dropbox.com/s/zfc8bx0q41bzvn1/open-vm-tools-9.2.2-4.fc18.src.rpm

Comment 23 Mohamed El Morabity 2013-04-10 05:31:26 UTC
Some comments:

- It looks like the help subpackage contains developement documentation. You'd better drop it and include it in the devel subpackage.

- Static libraries are not recommanded to be provided:
     http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries_2
Unless you have a good reason to provide them, please drop them. You can use the --disable-static option in %configure. Remove also all *.la files.

- Be careful not to own directories already owned by other packages: the desktop subpackage provides some desktop files in %{_sysconfdir}/xdg/autostart/, please refer to them in « %files desktop » section as below:
%files desktop
...
%{_sysconfdir}/xdg/autostart/*.desktop

...

Also in devel subpackage:
%files devel
...
%{_includedir}/vmGuestLib/

- Using %defattr(..) macro, after each %files section, is deprecated. Please drop all of them.

- Doc. files are usually installed in %{_docdir}/%{name}-%{version}. You can probably use the --docdir=%{_defaultdocdir}/%{name}-%{version}/ option in %configure, instead of installing API documentation « manually » through %doc.
Using %exclude is also discouraged to drop files from packages. You should remove them instead in %install section.

- The summary may be more explicit about the roles of the package: something like « Open VMware Tools for virtual machines hosted on VMware » or whatever may be more explicit for the main package.

- Last but not least, open-vm-tools are not usable without specific kernel modules, unless I'm wrong. Since Fedora doesn't allow inclusion of external kernel modules, are they plans from VMware to merge them in the vanilla kernel? Is there a schedule or a kernel version target?

Comment 24 Simone Caronni 2013-04-10 08:03:31 UTC
(In reply to comment #21)
> Sure. Could you please CC me on the ticket?

Sure.

> Thanks for making all the changes and providing the patch. Given all these
> complexities and testing effort, I'm inclined to focus on Fedora 18+ and
> RHEL 7+, because those are almost the same from systemd perspective. That
> will help me keep the SPEC file simple and testing is easy for me. Hope you
> would agree with me?

I can test and I'm offering myself as co-mantainer for the package, but I would like to have epel 5 and 6 available. Is that ok for you?

I'm writing the SysV init script for RHEL now.

> I looked at the bug https://bugs.kde.org/show_bug.cgi?id=190522 and it seems
> to have been fixed in a way that it is not reproducible. However, I will not
> be able to fix/test the note right now, because this file is coming from
> open-vm-tools source and we will need to fix the source code as sourceforge.
> I think we will need to raise a ticket for open-vm-tools code.

In the mean time you can provide a patch for it. I would rather not apply a fix for a problem we don't know it does exist.

People can always file a bug later if they have some problems running kde with the guest agent starting. From the bug report, the issue does not seem to happen since KDE 4.8.

> Removed RestartSec. TimeoutStopSec default is 90s which is way too long from
> service stop and guest shutdown perspectives. Please note that tools service
> does not handle SIGTERM nicely, so systemd ends up timing out and issues
> SIGKILL ultimately to kill this service. I believe this also needs to be
> fixed in open-vm-tools source code.

> I had started with "simple" service and had some issues. So, I set it to
> forking, I will try again with "simple".

Again, you can still patch the code and push it upstream.

(In reply to comment #23)
> - It looks like the help subpackage contains developement documentation.
> You'd better drop it and include it in the devel subpackage.

Agreed, this is api documentation and the help package is fairly small (100 kb).

> - Static libraries are not recommanded to be provided:
>
> Unless you have a good reason to provide them, please drop them. You can use
> the --disable-static option in %configure. Remove also all *.la files.

Yes, please delete them.

> - Be careful not to own directories already owned by other packages: the
> desktop subpackage provides some desktop files in
> %{_sysconfdir}/xdg/autostart/, please refer to them in « %files desktop »
> section as below:
> %files desktop
> ...
> %{_sysconfdir}/xdg/autostart/*.desktop

Yes, I did not notice this, please fix it as well.

> Also in devel subpackage:
> %files devel
> ...
> %{_includedir}/vmGuestLib/

The one which is now in place works fine as well as at the moment of packaging there are no extra files in /usr/include/ except those from the package, so the folder is included. You can change as suggested here, if you prefer.

> - Using %defattr(..) macro, after each %files section, is deprecated. Please
> drop all of them.

Isn't this needed for EPEL 5?

> - Doc. files are usually installed in %{_docdir}/%{name}-%{version}. You can
> probably use the --docdir=%{_defaultdocdir}/%{name}-%{version}/ option in
> %configure, instead of installing API documentation « manually » through
> %doc.

If you can, yes, please do.

> Using %exclude is also discouraged to drop files from packages. You should
> remove them instead in %install section.

I prefer this too, as while doing the actual building you can have a folder containing only what's needed in the package.
 
> - The summary may be more explicit about the roles of the package: something
> like « Open VMware Tools for virtual machines hosted on VMware » or whatever
> may be more explicit for the main package.

Yep.

> - Last but not least, open-vm-tools are not usable without specific kernel
> modules, unless I'm wrong. Since Fedora doesn't allow inclusion of external
> kernel modules, are they plans from VMware to merge them in the vanilla
> kernel? Is there a schedule or a kernel version target?

So far, as far as I know, all the required drivers except for the hgfs driver for filesystem mounting are included in RHEL 6.4+ and Fedora 17+. Do not know for RHEL 5. Any info on this?

Thanks Mohamed for your comments.

Comment 25 Mohamed El Morabity 2013-04-10 08:26:10 UTC
> > - Last but not least, open-vm-tools are not usable without specific kernel
> > modules, unless I'm wrong. Since Fedora doesn't allow inclusion of external
> > kernel modules, are they plans from VMware to merge them in the vanilla
> > kernel? Is there a schedule or a kernel version target?
> 
> So far, as far as I know, all the required drivers except for the hgfs
> driver for filesystem mounting are included in RHEL 6.4+ and Fedora 17+. Do
> not know for RHEL 5. Any info on this?
open-vm-tools provides also the following kernel modules:
- vmblock
- vmci
- vmhgfs
- vmsync
- vmxnet
- vsock
Their build is disabled here, for the reasons given above.
Only vsock is already available in vanilla kernel (>2.6.32, so not in RHEL6). vmxnet is not available, whereas vmxnet3 is in the kernel for a long time. Those two drivers don't support anyway the same virtual net devices.
vmblock is required to handle drag-and-drop file copy in WorkStation and Player.
open-vm-tools could integrate "as if" the Fedora repos, but its support would be very limited.

Comment 26 Simone Caronni 2013-04-10 12:46:46 UTC
(In reply to comment #25)
> open-vm-tools provides also the following kernel modules:
> - vmblock
> - vmci
> - vmhgfs
> - vmsync
> - vmxnet
> - vsock

I confirm what Mohamed says, I have only the following drivers "natively" in my Fedora and RHEL systems, as also noted on the RHEL 6.4 Technical notes [1]:

- vmxnet3
- vmw_pvscsi
- vmware_balloon
- vmmouse_drv
- vmware_drv

> Only vsock is already available in vanilla kernel (>2.6.32, so not in
> RHEL6).

Yes, it has been first included in 3.9rc1 [2]. So it will be in Fedora 19 first and backported to Fedora 18 3.9.x kernels. It might be that eventually it will make its way into RHEL.

> open-vm-tools could integrate "as if" the Fedora repos, but its support
> would be very limited.

I agree, but I'm also convinced that this should go in the repositories. When new kernel modules will be integrated, the functionality will start working.

I'm installing it right now the latest spec file on some Fedora guests in our environment, will let you know what can I do with it. If at least it can fill the guest information in the VMware console like ip address, shutdown, etc. I think it's a big win. Both for Fedora and RHEL.

What do you think?

Will let you have some feedback in a few minutes.



[1] https://access.redhat.com/site/documentation/en-US/Red_Hat_Enterprise_Linux/6/html-single/6.4_Release_Notes/index.html#vmware

[2] https://www.kernel.org/pub/linux/kernel/projects/backports/stable/v3.9-rc1/ChangeLog-3.9-rc1-3

Comment 27 Simone Caronni 2013-04-10 13:19:20 UTC
(In reply to comment #26)
> (In reply to comment #25)
> > open-vm-tools provides also the following kernel modules:
> > - vmblock
> > - vmci
> > - vmhgfs
> > - vmsync
> > - vmxnet
> > - vsock
> 
> I confirm what Mohamed says, I have only the following drivers "natively" in
> my Fedora and RHEL systems, as also noted on the RHEL 6.4 Technical notes
> [1]:
> 
> - vmxnet3
> - vmw_pvscsi
> - vmware_balloon
> - vmmouse_drv
> - vmware_drv

I forgot to add vmwgfx in Fedora.

Comment 28 Simone Caronni 2013-04-10 14:35:32 UTC
I installed the tools in some Fedora 17 and 18 guests, I have IP information reporting, Guest OS (it is named Fedora! Yay!) and I can power off / restart the guest cleanly with the vCenter console.

I think it's a great addition even with an incomplete set of drivers. One could always opt to use the source tarball in VMware ESX or the VMware official packages (http://www.vmware.com/download/packages.html)

Adding RHEL is not much effort and I'm offering myself as co-mantainer.

Here is an updated package along with a patch to your previous file. Included is:

- The SysV init script
- RHEL conditionals for 5 and 6
- A text file with the description of the drivers needed / used
- Renamed the service and Sysv init script from "open-vm-tools-guestd" to "vmtoolsd", the command is really too long and not intuitive
- %defattr is still there as I don't know if it's needed for RHEL 5.

SPEC File URL: http://slaanesh.fedorapeople.org/open-vm-tools.spec
SRPM URL: http://slaanesh.fedorapeople.org/open-vm-tools-9.2.2-5.fc18.src.rpm

patch from previous spec file: http://slaanesh.fedorapeople.org/open-vm-tools.spec.patch

text file with driver status: http://slaanesh.fedorapeople.org/open-vm-tools-drivers.txt

Can you give me some feedback?

Thanks,
--Simone

Comment 29 Ravindra Kumar 2013-04-10 22:24:11 UTC
Thanks for the review Mohamed, and thanks Simone for testing things and providing patch for Mohamed's comments.

> - Last but not least, open-vm-tools are not usable without specific kernel
> modules, unless I'm wrong.

This is not entirely accurate. open-vm-tools provide lots of administrative and customization capabilities of the VM from the VM management interface. The confusion comes from open-vm-tools code having drivers code as well, but actually most of the drivers are required for specific devices and not for the open-vm-tools operation itself. The open-vm-tools user space components that we are packaging in this bug are dependent on only two drivers vmhgfs and vmblock. vmhgfs is needed for "Shared Folder" functionality in Workstation and vmblock driver is needed for "Drag and Drop" feature. For vmblock driver, the alternative is to use Fuse and that is what we are doing in this package, so we are fine with "Drag and Drop". The only missing piece of the puzzle is vmhgfs driver, which will impact the "Shared Folder" functionality of Workstation. And, with no upstreaming planned for this driver, users will have to either use VMware Tools provided by VMware or build and install this driver from open-vm-tools source code.

> Since Fedora doesn't allow inclusion of external kernel modules, are they
> plans from VMware to merge them in the vanilla kernel?
> Is there a schedule or a kernel version target?

Most of the kernel modules except vmhgfs are already part of Linux Kernel 3.6 and even older ones like 2.6.32+.

> So far, as far as I know, all the required drivers except for the hgfs driver
> for filesystem mounting are included in RHEL 6.4+ and Fedora 17+. Do not know
> for RHEL 5. Any info on this?

RHEL 5 uses Linux Kernel 2.6.18 and it is missing most of the drivers. May be
all of them. Most of the upstreaming has been done in 2.6.32 and later kernels.

> I can test and I'm offering myself as co-mantainer for the package, but I would like to have epel 5 and 6 available. Is that ok for you?

I will integrate your changes with a condition that we will be testing and supporting only Fedora 19+ and RHEL 7+. All older releases will have to be tested and verified by you or someone who is going to use the RPM on those distros.

> I'm installing it right now the latest spec file on some Fedora guests in our environment, will let you know what can I do with it. If at least it can fill the guest information in the VMware console like ip address, shutdown, etc. I think it's a big win. Both for Fedora and RHEL.
> What do you think?

That should work normally for most of the administrative operations.

I'm integrating your changes and will publish the files soon with my changes.

Comment 30 Ravindra Kumar 2013-04-10 22:41:07 UTC
Simone, I have two questions.

1. Why do we need systemd-sysv for these releases?

 47 %if 0%{?fedora} >= 18 || 0%{?rhel} >= 7
 48 Requires(post):         systemd-sysv

2. I'm going to drop drivers.txt file (explanation in more previous comment).
Are you ok with that?

Comment 31 Ravindra Kumar 2013-04-11 00:01:19 UTC
Created attachment 733937 [details]
Modified open-vm-tools.init

Patch to Simone's init script.

Comment 32 Ravindra Kumar 2013-04-11 00:02:51 UTC
The patch I have provided for init script is for following reason.

I did not find the status check before starting the service, so I have also modified the init script with status checks and missing exist $? at the end. Now it is more like the template provided here: https://fedoraproject.org/wiki/Packaging:SysVInitScript#Initscript_template.

I will upload the spec file and srpm in a short while.

Comment 33 Ravindra Kumar 2013-04-11 01:30:52 UTC
> - Doc. files are usually installed in %{_docdir}/%{name}-%{version}. You can probably use the --docdir=%{_defaultdocdir}/%{name}-%{version}/ option in %configure, instead of installing API documentation « manually » through %doc.

This is causing inclusion of help files in base package as well as in devel
package. I will have to revert this change to avoid the issue.

Comment 34 Simone Caronni 2013-04-11 07:21:38 UTC
(In reply to comment #29)
> > I can test and I'm offering myself as co-mantainer for the package, but I would like to have epel 5 and 6 available. Is that ok for you?
> 
> I will integrate your changes with a condition that we will be testing and
> supporting only Fedora 19+ and RHEL 7+. All older releases will have to be
> tested and verified by you or someone who is going to use the RPM on those
> distros.

To me is fine, you can add me as co-mantainer or make me directly the owner of the el5, el6, f17, f18 branches.


(In reply to comment #30)
> 1. Why do we need systemd-sysv for these releases?
> 
>  47 %if 0%{?fedora} >= 18 || 0%{?rhel} >= 7
>  48 Requires(post):         systemd-sysv

Sorry, it was a leftover from another spec file from which I did copy the text. It will probably be needed at rhel 7 release time to make the upgrade from RHEL 6 (SysV) to the RHEL 7 (systemd) package.
I will add it to the RHEL 6 branch when it will be time.

> 2. I'm going to drop drivers.txt file (explanation in more previous comment).
> Are you ok with that?

Well, I would prefer that a text file goes along with the package, I think it would be better to leave that in.

I'm pretty sure someone will open a bug saying that the required modules are missing or another discussion will rise. It's clear for you (and now all the people here in CC) but not for people looking for a way to install the tools in Fedora et al.

Maybe you can expand it adding the explanation you did on comment #29. That would be good information, along the kernel drivers information.

(In reply to comment #32)
> The patch I have provided for init script is for following reason.
> 
> I did not find the status check before starting the service, so I have also
> modified the init script with status checks and missing exist $? at the end.
> Now it is more like the template provided here:
> https://fedoraproject.org/wiki/Packaging:SysVInitScript#Initscript_template.

Yes, please integrate, probably rpmdev-newspec is not on par with the Packaging Guidelines.

(In reply to comment #33)
> > - Doc. files are usually installed in %{_docdir}/%{name}-%{version}. You can probably use the --docdir=%{_defaultdocdir}/%{name}-%{version}/ option in %configure, instead of installing API documentation « manually » through %doc.
> 
> This is causing inclusion of help files in base package as well as in devel
> package. I will have to revert this change to avoid the issue.

Yes, but please add api docs only to the devel subpackage and not in a separate help subpackage.

Thanks,
--Simone

Comment 35 Simone Caronni 2013-04-11 07:43:59 UTC
Ravindra, I've created the FESco ticket for the boot enabled service. I've put you in CC and referenced it here.

Comment 36 Richard W.M. Jones 2013-04-11 07:53:31 UTC
I have sponsored Ravindra Kumar into the packager group.

Comment 37 Simone Caronni 2013-04-11 07:57:24 UTC
I've discovered a bug in RHEL 6 official packaging, referencing it here. It prevents building by using "procps-devel".

I've put it as a blocker, but this is only relevant to RHEL 6.

Comment 38 Ravindra Kumar 2013-04-11 08:58:04 UTC
> I have sponsored Ravindra Kumar into the packager group.

Thanks Richard.

> Sorry, it was a leftover from another spec file from which I did copy the text.
> It will probably be needed at rhel 7 release time to make the upgrade from
> RHEL 6 (SysV) to the RHEL 7 (systemd) package.
> I will add it to the RHEL 6 branch when it will be time.

Sure.

> Well, I would prefer that a text file goes along with the package, I think it
> would be better to leave that in.

How about fixing it in the README file from open-vm-tools source? That way it
will stay more accurate.

> Yes, but please add api docs only to the devel subpackage and not in a separate
> help subpackage.

Sure. I'm keeping most of your changes. I was not right --docdir is not
causing double packaging of help, it seems to be something else. I'm
still trying to figure it out.

> I've created the FESco ticket for the boot enabled service.

Thanks for creating this ticket.

> I've discovered a bug in RHEL 6 official packaging, referencing it here. It
> prevents building by using "procps-devel".

In your bug, you have mentioned that you are able to work around it using yum.
Is that so?

Comment 39 Simone Caronni 2013-04-11 09:11:46 UTC
(In reply to comment #38)
> How about fixing it in the README file from open-vm-tools source? That way it
> will stay more accurate.

Sure, no problem; as long as the information is available in the package I'm fine. Are you patching the README file?
 
> Sure. I'm keeping most of your changes. I was not right --docdir is not
> causing double packaging of help, it seems to be something else. I'm
> still trying to figure it out.

Ok.

> In your bug, you have mentioned that you are able to work around it using
> yum.
> Is that so?

Not exactly. If you install on a system both procps and procps-devel through yum, you don't have the error as ldconfig recreates the symlink.

Mock and Koji are more picky and refuse to install both packages specifying that the files conflict. Koji is required for building release packages.

The only way to build with koji or mock is to change momentarily the BuildRequires to procps. That's not a problem, the RHEL 6 build can follow later.

Comment 40 Ravindra Kumar 2013-04-11 09:42:38 UTC
> Sure. I'm keeping most of your changes. I was not right --docdir is not
> causing double packaging of help, it seems to be something else. I'm
> still trying to figure it out.

In order to make progress with this review. I've uploaded the latest SPEC file
and SRPM. I will continue to debug the double inclusion of documentation files
and fix it once I find it out.

SPEC File URL: https://www.dropbox.com/s/68yvjzfbttl4gt9/open-vm-tools.spec
SRPM URL: https://www.dropbox.com/s/lguwpdfldxpagca/open-vm-tools-9.2.2-6.fc18.src.rpm

> Are you patching the README file?

Not right away. I would like to get this package in with the current open-vm-tools. Subsequently, I will make patches for open-vm-tools changes. Then we can
push updates to Fedora again.

Comment 41 Richard W.M. Jones 2013-04-11 10:48:46 UTC
I also looked over the spec file and it seems sane to me.  There
are no obvious problems.

My personal preference would be to remove the conditions on
different versions of Fedora/RHEL and just have a separate spec
file for each branch, which would also allow the spec file to
be modernized for Fedora.  However this is entirely up to the
preferences of the packager/maintainer.

docdir handling seems a bit unothodox, but if it works then
it's fine.

Comment 42 Simone Caronni 2013-04-11 11:33:44 UTC
(In reply to comment #41)
> docdir handling seems a bit unothodox, but if it works then
> it's fine.

Me too, I don't really like it, and it doesn't work for the devel package, even if you change the directory where --docdir points to. Seems to work fine if you include the docs in the main package.

Please apply the following patch, it simplifies the docs packaging and removes another %exclude from the file lists.

--- open-vm-tools.spec.old	2013-04-11 13:24:04.988138217 +0200
+++ open-vm-tools.spec	2013-04-11 13:24:18.796410870 +0200
@@ -99,8 +99,7 @@
 %configure \
     --without-kernel-modules \
     --disable-static \
-    --enable-docs \
-    --docdir=%{_defaultdocdir}/%{name}-devel-%{version}/
+    --enable-docs
 make %{?_smp_mflags}
 
 %install
@@ -108,7 +107,8 @@
 make install DESTDIR=%{buildroot}
 
 # Remove unnecessary .la files
-rm %{buildroot}/%{_libdir}/*.la
+find %{buildroot} -name '*.la' -delete
+rm -fr %{buildroot}%{_defaultdocdir}
 
 %if 0%{?fedora} || 0%{?rhel} >= 7
 
@@ -200,7 +200,6 @@
 %dir %{_libdir}/%{name}/plugins
 %dir %{_libdir}/%{name}/plugins/common
 %{_libdir}/%{name}/plugins/common/*.so
-%exclude %{_libdir}/%{name}/plugins/common/*.la
 %dir %{_libdir}/%{name}/plugins/vmsvc
 %{_libdir}/%{name}/plugins/vmsvc/*.so
 %{_sbindir}/mount.vmhgfs
@@ -221,7 +220,7 @@
 
 %files devel
 %defattr(-,root,root,-)
-%doc
+%doc docs/api/build/*
 %{_includedir}/vmGuestLib/
 %{_libdir}/pkgconfig/*.pc
 %{_libdir}/libguestlib.so

> My personal preference would be to remove the conditions on
> different versions of Fedora/RHEL and just have a separate spec
> file for each branch, which would also allow the spec file to
> be modernized for Fedora.  However this is entirely up to the
> preferences of the packager/maintainer.

I have the exact opposite feeling regarding this, if I mantain more branches and there's no reason that they should not be kept in sync I prefer to have them merged with master so I can apply/push the same fix.

The Fedora 17 blocks will go away with the release of Fedora 19 in a few months.

But since I will probably be the one mantaining the other branches (el5, el6, f17, f18) if it's ok for everybody we can review this and at the time of initial committing to the repositories Ravindra can push a clean spec for Fedora 19 and I will push the other one in the other branches.

Can I proceed with the formal review once Ravindra applies the aforementioned patch?

Comment 43 Simone Caronni 2013-04-11 11:36:33 UTC
Erhm, just discovered a font packaged with the docs:

/usr/share/doc/open-vm-tools-devel-9.2.2/html/FreeSans.ttf

This should be removed as well. Updated patch:

--- open-vm-tools.spec.old	2013-04-11 13:24:04.988138217 +0200
+++ open-vm-tools.spec	2013-04-11 13:36:06.518425057 +0200
@@ -99,8 +99,7 @@
 %configure \
     --without-kernel-modules \
     --disable-static \
-    --enable-docs \
-    --docdir=%{_defaultdocdir}/%{name}-devel-%{version}/
+    --enable-docs
 make %{?_smp_mflags}
 
 %install
@@ -108,7 +107,9 @@
 make install DESTDIR=%{buildroot}
 
 # Remove unnecessary .la files
-rm %{buildroot}/%{_libdir}/*.la
+find %{buildroot} -name '*.la' -delete
+find %{buildroot} -name '*.ttf' -delete
+rm -fr %{buildroot}%{_defaultdocdir}
 
 %if 0%{?fedora} || 0%{?rhel} >= 7
 
@@ -200,7 +201,6 @@
 %dir %{_libdir}/%{name}/plugins
 %dir %{_libdir}/%{name}/plugins/common
 %{_libdir}/%{name}/plugins/common/*.so
-%exclude %{_libdir}/%{name}/plugins/common/*.la
 %dir %{_libdir}/%{name}/plugins/vmsvc
 %{_libdir}/%{name}/plugins/vmsvc/*.so
 %{_sbindir}/mount.vmhgfs
@@ -221,7 +221,7 @@
 
 %files devel
 %defattr(-,root,root,-)
-%doc
+%doc docs/api/build/*
 %{_includedir}/vmGuestLib/
 %{_libdir}/pkgconfig/*.pc
 %{_libdir}/libguestlib.so

Comment 44 Richard W.M. Jones 2013-04-11 11:50:12 UTC
(In reply to comment #42)
> Can I proceed with the formal review once Ravindra applies the
> aforementioned patch?

Please do.

Comment 45 Ravindra Kumar 2013-04-12 00:31:45 UTC
> But since I will probably be the one mantaining the other branches (el5, el6,
> f17, f18) if it's ok for everybody we can review this and at the time of
> initial committing to the repositories Ravindra can push a clean spec for
> Fedora 19 and I will push the other one in the other branches.

This sounds like a good plan.

> Erhm, just discovered a font packaged with the docs:
> /usr/share/doc/open-vm-tools-devel-9.2.2/html/FreeSans.ttf
> +find %{buildroot} -name '*.ttf' -delete

Your fix does not seem to address it because we are packaging the doc files
directly from the docs/api/build and not from buildroot. I think we will have
to remove this file from open-vm-tools source itself.

Simone, your following patch solved the issue of inclusion of help files in
the base package:
> +rm -fr %{buildroot}%{_defaultdocdir}

Thanks for fixing it for me :-)

Here are the patched and updated files:

SPEC File URL: https://www.dropbox.com/s/0hkmz9smo13f46k/open-vm-tools.spec
SRPM URL: https://www.dropbox.com/s/c7cfb6m16pul5gi/open-vm-tools-9.2.2-7.fc18.src.rpm

Comment 46 Simone Caronni 2013-04-12 07:09:15 UTC
For the font, you need also to delete it from the current release; when you'll be updating the package upstream and thus generate a new package, you can remove the line.

--- open-vm-tools.spec.old	2013-04-12 08:46:27.422858942 +0200
+++ open-vm-tools.spec	2013-04-12 08:59:51.521904690 +0200
@@ -85,6 +85,7 @@
 
 %prep
 %setup -q -n %{name}-%{version}-%{toolsbuild}
+rm -f docs/api/build/FreeSans.ttf
 
 %build
 # open-vm-tools source has some warnings generated because

Let's wait a moment on the FESCo decision on whether to start it at boot or not in Fedora. If such is the case; here is the patch for Fedora 17:

--- open-vm-tools.spec.old	2013-04-12 08:46:27.422858942 +0200
+++ open-vm-tools.spec	2013-04-12 08:47:09.976766695 +0200
@@ -129,7 +129,7 @@
 /usr/sbin/ldconfig
 if [ $1 -eq 1 ] ; then
     # Initial installation
-    /bin/systemctl daemon-reload >/dev/null 2>&1 || :
+    /bin/systemctl enable %{toolsdaemon}.service >/dev/null 2>&1 || :
 fi
 
 %preun

For Fedora 18 the systemd macro does not change and vmtoolsd needs to be added to the presets (already asked on FESCo ticket).

Comment 47 Simone Caronni 2013-04-12 07:18:46 UTC
(In reply to comment #45)
> I think we will have to remove this file from open-vm-tools source itself.

The main page of the project gives the idea that is totally abandoned, showing releases and VMWare events from 2010.

While on the contrary releases have been frequent, with the one currently reviewed here being only 4 months old:

http://sourceforge.net/projects/open-vm-tools/files/open-vm-tools/stable-9.2.x/

When creating the new source release for the removal of the ttf file and adding the kernel modules information to the README file, can I ask you to update the website? :D

Thanks.

Comment 48 Richard W.M. Jones 2013-04-12 08:17:23 UTC
FYI:

The ttf file is created by doxygen.  It's not part of
open-vm-tools, but copied from /usr/share/fonts/gnu-free
from the existing system (package 'gnu-free-sans-fonts' on Fedora).

Furthermore the doxygen.css file uses this font (although
it has fallbacks).

Comment 49 Simone Caronni 2013-04-12 08:31:25 UTC
Oops, you're right, the rm line should be in the %build or %install section.

--- open-vm-tools.spec.old	2013-04-12 08:46:27.422858942 +0200
+++ open-vm-tools.spec	2013-04-12 10:29:41.633744622 +0200
@@ -107,6 +107,7 @@
 # Remove unnecessary files from packaging
 find %{buildroot}/%{_libdir} -name '*.la' -delete
 rm -fr %{buildroot}/%{_defaultdocdir}
+rm -f docs/api/build/FreeSans.ttf
 
 %if 0%{?fedora} || 0%{?rhel} >= 7
 
@@ -129,7 +130,7 @@
 /usr/sbin/ldconfig
 if [ $1 -eq 1 ] ; then
     # Initial installation
-    /bin/systemctl daemon-reload >/dev/null 2>&1 || :
+    /bin/systemctl enable %{toolsdaemon}.service >/dev/null 2>&1 || :
 fi
 
 %preun

btw Fedora already has that font by default in package gnu-free-sans-fonts.

Should we add gnu-free-sans-fonts as BuildRequires? Koji does not give any error when building the current package.

Comment 50 Richard W.M. Jones 2013-04-12 08:41:29 UTC
If Koji doesn't give an error, then don't worry about it.

Comment 51 Simone Caronni 2013-04-12 08:55:18 UTC
Off Topic: Ravindra, I don't have any problem on the systems I tested onto; and the code seems in good shape, but is anybody taking seriously the sourceforge pages and website?

There are 170 bugs reported for the tools and no one is assigned:

http://sourceforge.net/tracker/?group_id=204462&atid=989708

Comment 52 Ravindra Kumar 2013-04-12 17:31:18 UTC
I had noticed that web site has gone bit old, but the main reason for that is
there are not many new exciting things there to update. Once we package it
in Fedora and RHEL, we will have things to update on the web site.

Regarding bugs, all 170 are not open bugs. If you filter by "open" status, you
would find much less of them.

Richard, if doxygen is copying the font, do we really need to remove the font
file? I would prefer to leave it there if doxygen wants it.

Comment 53 Richard W.M. Jones 2013-04-12 18:19:43 UTC
The font file seems fine.  I would not remove it.

Comment 54 Simone Caronni 2013-04-13 12:16:17 UTC
(In reply to comment #53)
> The font file seems fine.  I would not remove it.

Really? Are you sure? I've never seen a package copying a file arbitrarily from the system and including it. I don't think this is allowed.

And if you remove the font file there's no harm as well, as the font is already included in the distribution. I tried removing the file and there's no change, the browser uses the system installed one.

Comment 55 Richard W.M. Jones 2013-04-13 12:54:07 UTC
It's not clear to me.  I asked on the packagers list:

http://lists.fedoraproject.org/pipermail/packaging/2013-April/009046.html

Comment 56 Nicolas Mailhot 2013-04-14 12:03:31 UTC
(In reply to comment #54)
> (In reply to comment #53)
> > The font file seems fine.  I would not remove it.
> 
> Really? Are you sure? I've never seen a package copying a file arbitrarily
> from the system and including it. I don't think this is allowed.


From:
https://fedoraproject.org/wiki/Packaging:FontsPolicy

1. You're not allowed to package fonts in the same (sub)package as other things

> Packagers MUST package each font family in a separate (noarch.rpm)
> (sub)package

2. You're not allowed to select the font file location yourself

> Creating font packages or subpackages in Fedora is done using the
> fontpackages-devel package

(where font location is determined by a macro precisely to avoid people putting fonts everywhere on the FS)

If you apply those mandatory rules in our packaging guidelines you end up with a freesans rpm clone. At which point (that the guidelines do not state explicitely, because it should be obvious) you're better of using the freesans rpm directly

In practical terms, those rules means that any non-font (sub)package if Fedora is only allowed to contain references or symlinks to the contents of a font (sub)package containing the font files they use

Comment 57 Ravindra Kumar 2013-04-16 00:51:24 UTC
Based on the discussion above, I have removed the font file from the packaging.
I could not remove it by inserting rm command in the %install and %build sections.
I had to insert the rm command in %check section to achieve this.

Here are the updated SPEC file and SRPM:

SPEC File URL: https://www.dropbox.com/s/158ss3zzpckaifp/open-vm-tools.spec
SRPM URL: https://www.dropbox.com/s/9fmzh8jkjoazrer/open-vm-tools-9.2.2-8.fc18.src.rpm

Comment 58 Richard W.M. Jones 2013-04-16 07:27:27 UTC
You mustn't do this in the %check section.

The reason you couldn't remove it in %install is likely because
%doc is copied after %install.  However you can still remove it
by doing (in %install):

rm docs/api/build/FreeSans.ttf

Comment 59 Simone Caronni 2013-04-16 07:48:29 UTC
Sorry, I'll be in a meeting all day, I'll start the official review later today or tomorrow.

Comment 60 Ravindra Kumar 2013-04-16 19:05:43 UTC
> However you can still remove it by doing (in %install):
> rm docs/api/build/FreeSans.ttf

Richard, I had this earlier from Simone's patch and it did not work because 'make
install' places it under 'docs/api/build/html/FreeSans.ttf' dir. I corrected the
path and rpm got generated but still with the font file!

Apparently, there is something after 'make install' that is copying the font
file again. I guessed it to be 'make check' and therefore I debugged it and
confirmed it is 'make check' that regenerates this file. I think following
are few possible ways to fix it:

1. Place 'rm docs/api/build/html/FreeSans.ttf' right after 'make check'
2. Copy selective doc files and exclude this font file under %files
3. Fix the open-vm-tools 'make check' target [not sure if this is feasible]

Comment 61 Richard W.M. Jones 2013-04-16 19:27:06 UTC
Your analysis must be wrong somewhere.  %check runs after
the RPM has been built, so it shouldn't matter if %check runs
doxygen again.

It's quite possible that 'make install' runs doxygen (it would
be an upstream bug).  However you can just remove the file
after 'make install', but not in %check.

Comment 62 Simone Caronni 2013-04-16 20:13:57 UTC
You have replaced the spec file on dropbox and not in the src.rpm file, please revert it as fedora-review parses this bug entry for downloading files.

This spec file does not even build a src.rpm as you used the %build, %check etc. tags in the comments; if you want to use those in the %changelog prepend them with another %; like %%build or %%check.

Apart from this, building your last src.rpm file on a Fedora 18 x86_64 and a RHEL 6 x86_64 mock chroot does not include the font file; so it seems it's fine as it is; maybe it's just some confusion :)

Building on Fedora 19 x86_64 produces this error and the build fails:

DEBUG: /builddir/build/BUILD/open-vm-tools-9.2.2-893683/lib/include/backdoor_types.h:125:164: error: typedef 'AssertOnCompileFailed' locally defined but not used [-Werror=unused-local-typedefs]

For me, the src.rpm file you produced is ok as is.

Comment 63 Ravindra Kumar 2013-04-16 20:54:17 UTC
> Your analysis must be wrong somewhere.  %check runs after
> the RPM has been built, so it shouldn't matter if %check runs
> doxygen again.

I have verified this with 'rpmbuild --nocheck' as well. I have done following
analysis with the SPEC file from dropbox (please note that this SPEC file has
a step to remove font file in %install section as well as in %check section):

1. I remove the step to remove font file 'rm docs/api/build/html/FreeSans.ttf' from %check section, the font file gets included in the devel rpm.
2. I inserted debug checks to see existence of this file before and after
'make check' and found that file was not present before 'make check', but it
was present after 'make check'.
3. Running rpmbuild with '--nocheck' does not include the font file in the
package.

> It's quite possible that 'make install' runs doxygen (it would
> be an upstream bug).  However you can just remove the file
> after 'make install', but not in %check.

I have done it already in the SPEC file present on dropbox as explained above.

> You have replaced the spec file on dropbox and not in the src.rpm file, please
> revert it as fedora-review parses this bug entry for downloading files.

My bad, I corrected it. While copying files between machines, I ended up with
wrong SPEC file there. Now, I have uploaded the right SPEC file, which was
used to generate the SRPM.

> Apart from this, building your last src.rpm file on a Fedora 18 x86_64 and a
> RHEL 6 x86_64 mock chroot does not include the font file; so it seems it's
> fine as it is; maybe it's just some confusion :)

I believe this is because I have 'rm docs/api/build/html/FreeSans.ttf' in %check
section. You can see it in the SPEC file inside SRPM.

> This spec file does not even build a src.rpm as you used the %build, %check
> etc. tags in the comments; if you want to use those in the %changelog
> prepend them with another %; like %%build or %%check.

Yeah, it is because I uploaded the wrong SPEC file.

> Building on Fedora 19 x86_64 produces this error and the build fails:

I will try Fedora 19 build today.

Comment 64 Simone Caronni 2013-04-16 21:06:53 UTC
The %check section is a nice addition but is not mandatory, if this cause more problems than anything we can just get rid of it.

Richard what you think?

Comment 65 Richard W.M. Jones 2013-04-16 21:14:05 UTC
TBH I'm confused about which version I'm supposed to be looking
at.  Also I don't think dropbox is a very good service -- the
MIME type it uses doesn't let me simply look at the file in the
browser.

Can you [Ravinda] please post the latest spec file.

Comment 66 Ravindra Kumar 2013-04-16 21:25:40 UTC
Current version is 8. As I updated in my yesterday's update, these are the URLs:

SPEC File URL: https://www.dropbox.com/s/158ss3zzpckaifp/open-vm-tools.spec
SRPM URL: https://www.dropbox.com/s/9fmzh8jkjoazrer/open-vm-tools-9.2.2-8.fc18.src.rpm

FYI, I have fixed the wrong file issue what Simone mentioned in the same link.
So, the same links are good to look at.

> Also I don't think dropbox is a very good service -- the
> MIME type it uses doesn't let me simply look at the file in the
> browser.

I will see if I can fix it in dropbox. Otherwise, I might need to request for
space on fedoraproject.

Comment 67 Ravindra Kumar 2013-04-16 21:29:58 UTC
> I will see if I can fix it in dropbox.

I can't find a way to specify mime-type property for the file in dropbox :-(
Please let me know if having storage space on fedoraproject will help in 
reviewing spec file.

Comment 68 Richard W.M. Jones 2013-04-17 07:20:59 UTC
I built the latest spec file, first removing the 'rm' command
in the %check section, and there is no ttf file in the result.

So remove the rm in the %check section.

Apart from that, the spec file looks fine to me.

Comment 69 Simone Caronni 2013-04-17 07:31:14 UTC
Sa(In reply to comment #68)
> I built the latest spec file, first removing the 'rm' command
> in the %check section, and there is no ttf file in the result.
> 
> So remove the rm in the %check section.
> 
> Apart from that, the spec file looks fine to me.

Exactly same behaviour here.

Starting the review now, FESCo has not yet approved the exception though. Please be patient as I have another meeting :)

Comment 70 Ravindra Kumar 2013-04-17 07:45:31 UTC
Thanks Richard and Simone. I will do a totally fresh and new build and verify
this. Also, I will wait for your other review comments before I make this change
in my SPEC file, so that I can merge all the changes in one single update.

Comment 71 Simone Caronni 2013-04-17 07:50:31 UTC
I've put spec and src.rpm on fedorapeople.org, as dropbox does not work with Fedora review as it opens the download form and it bombs out.

If you are sponsored I think you can have already access to your fedorapeople.org space, btw.

%changelog
* Wed Apr 17 2013 Simone Caronni <negativo17@gmail.com> - 9.2.2-9
- Removed rm command in %%check section.
- Remove blank character at the beginning of each changelog line.

SPEC File URL: http://slaanesh.fedorapeople.org/open-vm-tools.spec
SRPM URL: http://slaanesh.fedorapeople.org/open-vm-tools-9.2.2-9.fc18.src.rpm

Comment 72 Simone Caronni 2013-04-17 09:07:23 UTC
Package Review
==============

Key:
[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
     Note: Unversioned so-files in private %_libdir subdirectory (see
     attachment). Verify they are not in ld path.
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf %{buildroot} present but not required
[x]: Sources contain only permissible code or content.
[x]: Each %files section contains %defattr if rpm < 4.4
     Note: %defattr present but not needed
[x]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[!]: Fully versioned dependency in subpackages, if present.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in open-vm-
     tools-desktop , open-vm-tools-devel
[x]: Package complies to the Packaging Guidelines
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "GPL", "CDDL", "LGPL", "CDDL BSD (3 clause) LGPL", "Unknown or
     generated", "BSD (3 clause) GPL", "MIT/X11 (BSD like)", "ISC", "CDDL
     LGPL", "CDDL GPL", "BSD (3 clause)", "GPL (v2 or later)", "GPL (v2)". 13
     files have unknown license. Detailed output of licensecheck in
     /home/slaanesh/Documents/fedora/905255-open-vm-tools/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: %config files are marked noreplace or the reason is justified.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[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.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

===== SHOULD items =====

Generic:
[!]: Spec use %global instead of %define.
     Note: %define majorversion 9.2 %define minorversion 2 %define toolsbuild
     893683 %define toolsversion %{majorversion}.%{minorversion} %define
     toolsdaemon vmtoolsd
[x]: Buildroot is not present
     Note: Buildroot: present but not needed
[!]: Avoid bundling fonts in non-fonts packages.
     Note: Package contains font files
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[!]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
     Note: Arch-ed rpms have a total of 1372160 bytes in /usr/share 1116160
     open-vm-tools-devel-9.2.2-9.fc18.x86_64.rpm 256000 open-vm-
     tools-9.2.2-9.fc18.x86_64.rpm
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.

Rpmlint
-------
Checking: open-vm-tools-9.2.2-9.fc18.x86_64.rpm
          open-vm-tools-desktop-9.2.2-9.fc18.x86_64.rpm
          open-vm-tools-devel-9.2.2-9.fc18.x86_64.rpm
open-vm-tools.x86_64: W: spelling-error %description -l en_US virtualization -> visualization, actualization, vitalization
open-vm-tools.x86_64: E: postin-without-ldconfig /usr/lib64/libvmtools.so.0.0.0
open-vm-tools.x86_64: E: postun-without-ldconfig /usr/lib64/libvmtools.so.0.0.0
open-vm-tools.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/open-vm-tools-9.2.2/README
open-vm-tools.x86_64: E: postin-without-ldconfig /usr/lib64/libguestlib.so.0.0.0
open-vm-tools.x86_64: E: postun-without-ldconfig /usr/lib64/libguestlib.so.0.0.0
open-vm-tools.x86_64: E: executable-marked-as-config-file /etc/pam.d/vmtoolsd
open-vm-tools.x86_64: E: script-without-shebang /etc/pam.d/vmtoolsd
open-vm-tools.x86_64: E: postin-without-ldconfig /usr/lib64/libhgfs.so.0.0.0
open-vm-tools.x86_64: E: postun-without-ldconfig /usr/lib64/libhgfs.so.0.0.0
open-vm-tools.x86_64: W: no-manual-page-for-binary vmware-xferlogs
open-vm-tools.x86_64: W: no-manual-page-for-binary vmware-checkvm
open-vm-tools.x86_64: W: no-manual-page-for-binary vmware-toolbox-cmd
open-vm-tools.x86_64: W: no-manual-page-for-binary vmware-hgfsclient
open-vm-tools.x86_64: W: no-manual-page-for-binary vmtoolsd
open-vm-tools.x86_64: W: no-manual-page-for-binary vmware-rpctool
open-vm-tools.x86_64: W: no-manual-page-for-binary mount.vmhgfs
open-vm-tools.x86_64: W: install-file-in-docs /usr/share/doc/open-vm-tools-9.2.2/INSTALL
open-vm-tools-desktop.x86_64: E: summary-too-long C User experience components for Open VMware Tools for virtual machines hosted on VMware
open-vm-tools-desktop.x86_64: W: no-documentation
open-vm-tools-desktop.x86_64: W: non-conffile-in-etc /etc/xdg/autostart/vmware-user.desktop
open-vm-tools-desktop.x86_64: W: no-manual-page-for-binary vmware-vmblock-fuse
open-vm-tools-desktop.x86_64: W: no-manual-page-for-binary vmware-user-suid-wrapper
3 packages and 0 specfiles checked; 9 errors, 14 warnings.

Rpmlint (installed packages)
----------------------------
# rpmlint open-vm-tools open-vm-tools-devel open-vm-tools-desktop
open-vm-tools.x86_64: W: spelling-error %description -l en_US virtualization -> visualization, actualization, vitalization
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libvmtools.so.0.0.0 /lib64/libcrypt.so.1
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libvmtools.so.0.0.0 /lib64/libicudata.so.49
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libvmtools.so.0.0.0 /lib64/libm.so.6
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/librt.so.1
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libcrypt.so.1
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libicui18n.so.49
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libicuuc.so.49
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libicudata.so.49
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libpthread.so.0
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libdl.so.2
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libm.so.6
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libglib-2.0.so.0
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libgthread-2.0.so.0
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/librt.so.1
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libcrypt.so.1
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libicui18n.so.49
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libicuuc.so.49
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libicudata.so.49
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libdl.so.2
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libm.so.6
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libglib-2.0.so.0
open-vm-tools.x86_64: E: postin-without-ldconfig /usr/lib64/libvmtools.so.0.0.0
open-vm-tools.x86_64: E: postun-without-ldconfig /usr/lib64/libvmtools.so.0.0.0
open-vm-tools.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/open-vm-tools-9.2.2/README
open-vm-tools.x86_64: E: postin-without-ldconfig /usr/lib64/libguestlib.so.0.0.0
open-vm-tools.x86_64: E: postun-without-ldconfig /usr/lib64/libguestlib.so.0.0.0
open-vm-tools.x86_64: E: executable-marked-as-config-file /etc/pam.d/vmtoolsd
open-vm-tools.x86_64: E: script-without-shebang /etc/pam.d/vmtoolsd
open-vm-tools.x86_64: E: postin-without-ldconfig /usr/lib64/libhgfs.so.0.0.0
open-vm-tools.x86_64: E: postun-without-ldconfig /usr/lib64/libhgfs.so.0.0.0
open-vm-tools.x86_64: W: no-manual-page-for-binary vmware-xferlogs
open-vm-tools.x86_64: W: no-manual-page-for-binary vmware-checkvm
open-vm-tools.x86_64: W: no-manual-page-for-binary vmware-toolbox-cmd
open-vm-tools.x86_64: W: no-manual-page-for-binary vmware-hgfsclient
open-vm-tools.x86_64: W: no-manual-page-for-binary vmtoolsd
open-vm-tools.x86_64: W: no-manual-page-for-binary vmware-rpctool
open-vm-tools.x86_64: W: no-manual-page-for-binary mount.vmhgfs
open-vm-tools.x86_64: W: install-file-in-docs /usr/share/doc/open-vm-tools-9.2.2/INSTALL
open-vm-tools-desktop.x86_64: E: summary-too-long C User experience components for Open VMware Tools for virtual machines hosted on VMware
open-vm-tools-desktop.x86_64: W: no-documentation
open-vm-tools-desktop.x86_64: W: non-conffile-in-etc /etc/xdg/autostart/vmware-user.desktop
open-vm-tools-desktop.x86_64: W: no-manual-page-for-binary vmware-vmblock-fuse
open-vm-tools-desktop.x86_64: W: no-manual-page-for-binary vmware-user-suid-wrapper
3 packages and 0 specfiles checked; 9 errors, 35 warnings.
# echo 'rpmlint-done:'

Requires
--------
open-vm-tools (rpmlib, GLIBC filtered):
    /bin/sh
    config(open-vm-tools)
    libc.so.6()(64bit)
    libcrypt.so.1()(64bit)
    libdl.so.2()(64bit)
    libdnet.so.1()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgcc_s.so.1(GCC_3.3)(64bit)
    libglib-2.0.so.0()(64bit)
    libgmodule-2.0.so.0()(64bit)
    libgobject-2.0.so.0()(64bit)
    libgthread-2.0.so.0()(64bit)
    libguestlib.so.0()(64bit)
    libhgfs.so.0()(64bit)
    libicudata.so.49()(64bit)
    libicui18n.so.49()(64bit)
    libicuuc.so.49()(64bit)
    libm.so.6()(64bit)
    libprocps.so.0()(64bit)
    libprocps.so.0(LIBPROCPS_0)(64bit)
    libpthread.so.0()(64bit)
    librt.so.1()(64bit)
    libstdc++.so.6()(64bit)
    libvmtools.so.0()(64bit)
    rtld(GNU_HASH)
    systemd

open-vm-tools-devel (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    libguestlib.so.0()(64bit)
    libhgfs.so.0()(64bit)
    libvmtools.so.0()(64bit)
    open-vm-tools

open-vm-tools-desktop (rpmlib, GLIBC filtered):
    libX11.so.6()(64bit)
    libXext.so.6()(64bit)
    libXi.so.6()(64bit)
    libXinerama.so.1()(64bit)
    libXrandr.so.2()(64bit)
    libXrender.so.1()(64bit)
    libXtst.so.6()(64bit)
    libatk-1.0.so.0()(64bit)
    libatkmm-1.6.so.1()(64bit)
    libc.so.6()(64bit)
    libcairo.so.2()(64bit)
    libcairomm-1.0.so.1()(64bit)
    libcrypt.so.1()(64bit)
    libdl.so.2()(64bit)
    libfontconfig.so.1()(64bit)
    libfreetype.so.6()(64bit)
    libfuse.so.2()(64bit)
    libfuse.so.2(FUSE_2.6)(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgdk-x11-2.0.so.0()(64bit)
    libgdk_pixbuf-2.0.so.0()(64bit)
    libgdkmm-2.4.so.1()(64bit)
    libgio-2.0.so.0()(64bit)
    libgiomm-2.4.so.1()(64bit)
    libglib-2.0.so.0()(64bit)
    libglibmm-2.4.so.1()(64bit)
    libgobject-2.0.so.0()(64bit)
    libgthread-2.0.so.0()(64bit)
    libgtk-x11-2.0.so.0()(64bit)
    libgtkmm-2.4.so.1()(64bit)
    libhgfs.so.0()(64bit)
    libicudata.so.49()(64bit)
    libicui18n.so.49()(64bit)
    libicuuc.so.49()(64bit)
    libm.so.6()(64bit)
    libpango-1.0.so.0()(64bit)
    libpangocairo-1.0.so.0()(64bit)
    libpangoft2-1.0.so.0()(64bit)
    libpangomm-1.4.so.1()(64bit)
    libpthread.so.0()(64bit)
    librt.so.1()(64bit)
    libsigc-2.0.so.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libvmtools.so.0()(64bit)
    open-vm-tools
    rtld(GNU_HASH)

Provides
--------
open-vm-tools:
    config(open-vm-tools)
    libguestInfo.so()(64bit)
    libguestlib.so.0()(64bit)
    libhgfs.so.0()(64bit)
    libhgfsServer.so()(64bit)
    libpowerOps.so()(64bit)
    libtimeSync.so()(64bit)
    libvix.so()(64bit)
    libvmbackup.so()(64bit)
    libvmtools.so.0()(64bit)
    open-vm-tools
    open-vm-tools(x86-64)

open-vm-tools-devel:
    open-vm-tools-devel
    open-vm-tools-devel(x86-64)
    pkgconfig(vmguestlib)

open-vm-tools-desktop:
    libdesktopEvents.so()(64bit)
    libdndcp.so()(64bit)
    libresolutionSet.so()(64bit)
    open-vm-tools-desktop
    open-vm-tools-desktop(x86-64)

Unversioned so-files
--------------------
open-vm-tools: /usr/lib64/open-vm-tools/plugins/common/libhgfsServer.so
open-vm-tools: /usr/lib64/open-vm-tools/plugins/common/libvix.so
open-vm-tools: /usr/lib64/open-vm-tools/plugins/vmsvc/libguestInfo.so
open-vm-tools: /usr/lib64/open-vm-tools/plugins/vmsvc/libpowerOps.so
open-vm-tools: /usr/lib64/open-vm-tools/plugins/vmsvc/libtimeSync.so
open-vm-tools: /usr/lib64/open-vm-tools/plugins/vmsvc/libvmbackup.so
open-vm-tools-desktop: /usr/lib64/open-vm-tools/plugins/vmusr/libdesktopEvents.so
open-vm-tools-desktop: /usr/lib64/open-vm-tools/plugins/vmusr/libdndcp.so
open-vm-tools-desktop: /usr/lib64/open-vm-tools/plugins/vmusr/libresolutionSet.so

Comment 73 Simone Caronni 2013-04-17 09:07:49 UTC
Not applicable issues:
======================

Unversioned so-files
--------------------
open-vm-tools: /usr/lib64/open-vm-tools/plugins/common/libhgfsServer.so
open-vm-tools: /usr/lib64/open-vm-tools/plugins/common/libvix.so
open-vm-tools: /usr/lib64/open-vm-tools/plugins/vmsvc/libguestInfo.so
open-vm-tools: /usr/lib64/open-vm-tools/plugins/vmsvc/libpowerOps.so
open-vm-tools: /usr/lib64/open-vm-tools/plugins/vmsvc/libtimeSync.so
open-vm-tools: /usr/lib64/open-vm-tools/plugins/vmsvc/libvmbackup.so
open-vm-tools-desktop: /usr/lib64/open-vm-tools/plugins/vmusr/libdesktopEvents.so
open-vm-tools-desktop: /usr/lib64/open-vm-tools/plugins/vmusr/libdndcp.so
open-vm-tools-desktop: /usr/lib64/open-vm-tools/plugins/vmusr/libresolutionSet.so

Ignore this, they are plugins and hidden in a subfolder without ldconfig configuration.

- update-desktop-database is invoked when required
  Note: desktop file(s) in open-vm-tools-desktop
  See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache
- Package installs a %{name}.desktop using desktop-file-install if there is
  such a file.
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#desktop

Ignore, this is not a real issue, as the desktop file is not an icon for the menu as it's an autostart.

- Spec file lacks Packager, Vendor, PreReq tags.
  Note: Found : Packager: Simone Caronni <negativo17@gmail.com>
  See: (this test has no URL)

Ignore, these should not be added.

- Large documentation must go in a -doc subpackage.
  Note: Documentation size is 1228800 bytes in 119 files.
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation

This is not applicable here, as the guidelines say the api documentation should go along with the -devel subpackage and it's not large.

Rpmlint (installed packages)
----------------------------
# rpmlint open-vm-tools open-vm-tools-devel open-vm-tools-desktop
open-vm-tools.x86_64: W: spelling-error %description -l en_US virtualization -> visualization, actualization, vitalization
open-vm-tools.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/open-vm-tools-9.2.2/README
open-vm-tools.x86_64: W: no-manual-page-for-binary vmware-xferlogs
open-vm-tools.x86_64: W: no-manual-page-for-binary vmware-checkvm
open-vm-tools.x86_64: W: no-manual-page-for-binary vmware-toolbox-cmd
open-vm-tools.x86_64: W: no-manual-page-for-binary vmware-hgfsclient
open-vm-tools.x86_64: W: no-manual-page-for-binary vmtoolsd
open-vm-tools.x86_64: W: no-manual-page-for-binary vmware-rpctool
open-vm-tools.x86_64: W: no-manual-page-for-binary mount.vmhgfs
open-vm-tools-desktop.x86_64: W: no-documentation
open-vm-tools-desktop.x86_64: W: non-conffile-in-etc /etc/xdg/autostart/vmware-user.desktop
open-vm-tools-desktop.x86_64: W: no-manual-page-for-binary vmware-vmblock-fuse
open-vm-tools-desktop.x86_64: W: no-manual-page-for-binary vmware-user-suid-wrapper

Ignore.

Comment 74 Simone Caronni 2013-04-17 09:08:02 UTC
Issues:
=======

- ldconfig called in %post and %postun if required.
  Note: /sbin/ldconfig not called in open-vm-tools
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Shared_Libraries

Please use /sbin/ldconfig as suggested so it works also with Epel.

[!]: Spec use %global instead of %define.
     Note: %define majorversion 9.2 %define minorversion 2 %define toolsbuild
     893683 %define toolsversion %{majorversion}.%{minorversion} %define
     toolsdaemon vmtoolsd

Please check this and apply where appropriate:
http://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define

[!]: Avoid bundling fonts in non-fonts packages.
     Note: Package contains font files

This happens to me again on the review! I've checked the Makefile in the docs folder; the check is rebuilding the docs.
So let's remove the %check section as it's not mandatory and causes more trouble than anything else.

[!]: Package should compile and build into binary rpms on all supported
     architectures.

Does not build on Fedora 19 with the error explained in comment #62.

open-vm-tools.x86_64: E: postin-without-ldconfig /usr/lib64/libvmtools.so.0.0.0
open-vm-tools.x86_64: E: postun-without-ldconfig /usr/lib64/libvmtools.so.0.0.0
open-vm-tools.x86_64: E: postin-without-ldconfig /usr/lib64/libguestlib.so.0.0.0
open-vm-tools.x86_64: E: postun-without-ldconfig /usr/lib64/libguestlib.so.0.0.0
open-vm-tools.x86_64: E: postin-without-ldconfig /usr/lib64/libhgfs.so.0.0.0
open-vm-tools.x86_64: E: postun-without-ldconfig /usr/lib64/libhgfs.so.0.0.0

Plase use /sbin/ldconfig as described above.

open-vm-tools.x86_64: E: executable-marked-as-config-file /etc/pam.d/vmtoolsd
open-vm-tools.x86_64: E: script-without-shebang /etc/pam.d/vmtoolsd

This should not be executable.

open-vm-tools.x86_64: W: install-file-in-docs /usr/share/doc/open-vm-tools-9.2.2/INSTALL

Please remove from %doc line.

open-vm-tools-desktop.x86_64: E: summary-too-long C User experience components for Open VMware Tools for virtual machines hosted on VMware

Please shorten (21 charachters maximum if I remember correctly).

open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libvmtools.so.0.0.0 /lib64/libcrypt.so.1
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libvmtools.so.0.0.0 /lib64/libicudata.so.49
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libvmtools.so.0.0.0 /lib64/libm.so.6
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/librt.so.1
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libcrypt.so.1
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libicui18n.so.49
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libicuuc.so.49
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libicudata.so.49
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libpthread.so.0
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libdl.so.2
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libm.so.6
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libguestlib.so.0.0.0 /lib64/libglib-2.0.so.0
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libgthread-2.0.so.0
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/librt.so.1
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libcrypt.so.1
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libicui18n.so.49
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libicuuc.so.49
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libicudata.so.49
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libdl.so.2
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libm.so.6
open-vm-tools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libhgfs.so.0.0.0 /lib64/libglib-2.0.so.0

Please see:
https://fedoraproject.org/wiki/Common_Rpmlint_issues#unused-direct-shlib-dependency

Comment 75 Simone Caronni 2013-04-17 09:08:28 UTC
License issues:
===============

I think we need some review here...

[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "GPL", "CDDL", "LGPL", "CDDL BSD (3 clause) LGPL", "Unknown or
     generated", "BSD (3 clause) GPL", "MIT/X11 (BSD like)", "ISC", "CDDL
     LGPL", "CDDL GPL", "BSD (3 clause)", "GPL (v2 or later)", "GPL (v2)". 13
     files have unknown license. Detailed output of licensecheck in
     /home/slaanesh/Documents/fedora/905255-open-vm-tools/licensecheck.txt

GPL
---
/var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-893683/modules/linux/vmci/linux/vmciKernelIf.c

CDDL
----
/var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-893683/modules/solaris/vmhgfs/filesystem.h

LGPL
----
/var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-893683/tests/testVmblock/manual-blocker.c

CDDL BSD (3 clause) LGPL
------------------------
/var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-893683/modules/shared/vmmemctl/backdoor_balloon.h

Unknown or generated
--------------------
/var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-893683/services/plugins/dndcp/dndGuest/dndCPTransportGuestRpc.hpp

BSD (3 clause) GPL
------------------
/var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-893683/modules/freebsd/shared/compat_mount.h

MIT/X11 (BSD like)
------------------
/var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-893683/services/plugins/resolutionSet/vmwarectrlproto.h

ISC
---
/var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-893683/lib/misc/base64.c

CDDL LGPL
---------
/var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-893683/lib/rpcOut/rpcout.c

CDDL GPL
--------
/var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-893683/modules/shared/vmxnet/vmnet_def.h

BSD (3 clause)
--------------
/var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-893683/modules/freebsd/vmblock/vfsops.c

GPL (v2 or later)
-----------------
/var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-893683/config/ltmain.sh

GPL (v2)
--------
/var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-893683/modules/linux/dkms.sh

Comment 76 Ravindra Kumar 2013-04-17 22:57:07 UTC
Thanks Simone for the detailed review. I'm working on these. I'm wondering if I
should remove old releases related bits from spec file before I submit it again
for next round of review? Or, should I do that right before final submission?

Comment 77 Simone Caronni 2013-04-18 07:38:12 UTC
Well, if it's going as in comment #42 that I will be the mantainer of the other branches, feel free to remove all the tags, I will readd them later.

Btw, FESCo has approved the exception for starting, I've asked to update the preset file for Fedora 18+.

I will add the correct systemd line to the Fedora 17 spec file,

Comment 78 Ravindra Kumar 2013-04-18 22:14:35 UTC
Simone, I have addressed all of your comments except license issue. Here are the
updated files.

SPEC File URL: https://www.dropbox.com/s/9f689yzcrnqf1vx/open-vm-tools.spec
SRPM URL: https://www.dropbox.com/s/leo2d05e9ln2cvd/open-vm-tools-9.2.2-10.fc18.src.rpm

I'm not able to test this using koji because I'm having trouble accessing koji
through a proxy.

Please let me know how to resolve the license issue.

> Well, if it's going as in comment #42 that I will be the mantainer of the other
> branches, feel free to remove all the tags, I will readd them later.

Koji scratch build also has some issues with %fedora usage, https://fedoraproject.org/wiki/Using_the_Koji_build_system#Scratch_Builds
May be this is one more reason to split these files. But, I'm fine to keep
the spec file as it is till review is going on.

> Btw, FESCo has approved the exception for starting, I've asked to update the
> preset file for Fedora 18+.

Thanks for taking care of this.

Comment 79 Simone Caronni 2013-04-19 07:30:17 UTC
(In reply to comment #78)
> Koji scratch build also has some issues with %fedora usage,
> https://fedoraproject.org/wiki/Using_the_Koji_build_system#Scratch_Builds
> May be this is one more reason to split these files. But, I'm fine to keep
> the spec file as it is till review is going on.

Yes, I usually build locally with mock as it's faster, and then submit the package. If the package fails to build you don't need necessarily to bump the release for a rebuild as if the package was built correctly, so it's pretty safe.

All the points have been fixed, so for me the package is good to go; Good job :)

We have only to solve the licensing issues. Have you had a change to read this?

https://fedoraproject.org/wiki/Licensing:FAQ

It lists common cases with multiple licensing scenarios. I think the best thing you could do is make another upstream release with the following:

- leverage licenses across the files
- fix the make check target
- fix the compilation problems
- add the driver notes to the README file as your comment #38

Bump the release and it will get approved. Otherwise since all the licenses seem compatible with each other please find a way to write it down in the License field in the spec file using the above link.

Comment 80 Dmitry Torokhov 2013-04-19 15:46:10 UTC
(In reply to comment #79)
> 
> We have only to solve the licensing issues. Have you had a change to read
> this?
> 
> https://fedoraproject.org/wiki/Licensing:FAQ
> 
> It lists common cases with multiple licensing scenarios. I think the best
> thing you could do is make another upstream release with the following:
> 
> - leverage licenses across the files

Simone,

At this time we are not considering changing the licenses on components of open-vm-tools project. The userspace and Linux kernel drivers are GPL, Freebsd kernel drivers are either GPL or BSD, and Solaris kernel drivers are CDDL.

As this particular package is for Linux userspace only the overall license on the package should be GPL (v2.1 only). 

Thanks,
Dmitry

Comment 81 Simone Caronni 2013-04-19 16:15:13 UTC
The source code release contains all those licenses though. Ignoring the kernel module files; this is the list of contained licenses:

> LGPL
> /var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-
> 893683/tests/testVmblock/manual-blocker.c

> MIT/X11 (BSD like)
> /var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-
> 893683/services/plugins/resolutionSet/vmwarectrlproto.h

> ISC
> /var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-
> 893683/lib/misc/base64.c

> CDDL LGPL
> /var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-
> 893683/lib/rpcOut/rpcout.c

> GPL (v2 or later)
> /var/lib/mock/fedora-18-x86_64/root/builddir/build/BUILD/open-vm-tools-9.2.2-
> 893683/config/ltmain.sh

Well, from what I see here:

https://fedoraproject.org/wiki/Licensing:FAQ#What_is_.22effective_license.22_and_do_I_need_to_know_that_for_the_License:_tag.3F

and here:

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios

it seems you can only leave GPLv2 as the License. Quoting the Packaging guidelines:

"The source code contains some .c files which are GPLv2+ and some other .c files which are BSD. They're compiled together to form an executable. Since some of the files are licensed as GPL, the resulting executable is also GPL. The License tag should read: License: GPLv2+
Note that you do NOT need to list BSD in the License tag, the License tag reflects the resulting, packaged, items in the binary RPM."

So, for my understanding your point seems valid and the License is valid as the BSD/MIT files are compatible with the GPL.

I'm not a licensing expert, though. Anyone has anything to say regarding this? Otherwise the package is good to go.

Thanks,
--Simone

Comment 82 Richard W.M. Jones 2013-04-19 16:29:50 UTC
Note:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#License:_field
that the License: field applies to the *binary*, not to the
source.

So if there are BSD / Solaris / whatever drivers in the open-vm-tools
source package, then the license of those has no bearing on the
License: field (provided AIUI that those drivers aren't being
compiled into the binary RPM).

Comment 83 Simone Caronni 2013-04-19 16:47:43 UTC
Perfect, thanks Richard.

Package approved!

Ravindra, please keep in touch when doing the inital pushing.

Thanks to everybody!

Comment 84 Simone Caronni 2013-04-19 16:58:32 UTC
I'll be off for the weekend and come back on monday (CET).

Comment 85 Ravindra Kumar 2013-04-19 21:39:26 UTC
Thanks Simone for reviewing this and working on this with me.

Assuming this review is complete from feedback perspective, I'm planning to
remove old branch checks and going to request only for f19 and future branches.
I believe Simone would be requesting for all other old branches, mainly f17 and
f18 etc.

Richard, do you have anything for me to look at before I start the SCM process?

Comment 86 Ravindra Kumar 2013-04-22 07:04:45 UTC
New Package SCM Request
=======================
Package Name: open-vm-tools
Short Description: Open VMware Tools for virtual machines hosted on VMware
Owners: ravindrakumar
Branches: f19
InitialCC: rjones slaanesh

Comment 87 Richard W.M. Jones 2013-04-22 07:31:58 UTC
(In reply to comment #85)
> Richard, do you have anything for me to look at before I start the SCM
> process?

No.

Comment 88 Gwyn Ciesla 2013-04-22 13:14:27 UTC
Git done (by process-git-requests).

Comment 89 Ravindra Kumar 2013-04-22 17:30:10 UTC
I have removed the steps related to old versions of Fedora and RHEL. Please
take a look at the final version of the SPEC file for Fedora 19 I would like
to checkin:

SPEC File URL: https://www.dropbox.com/s/hzs0lk8l1reyaix/open-vm-tools.spec
SRPM URL: https://www.dropbox.com/s/ilp61bz2ql64oa6/open-vm-tools-9.2.2-11.fc18.src.rpm

Comment 90 Ravindra Kumar 2013-04-23 23:36:23 UTC
Finally, I have green results from koji for the SRPM link I provided above
in my last update:

http://koji.fedoraproject.org/koji/taskinfo?taskID=5294959 (f19)
http://koji.fedoraproject.org/koji/taskinfo?taskID=5294970 (f20)

Comment 91 Ravindra Kumar 2013-04-24 00:09:08 UTC
Checked in the package to rawhide and f19 branches.

Here are the links for fedpkg builds:

http://koji.fedoraproject.org/koji/taskinfo?taskID=5294998 (rawhide)
http://koji.fedoraproject.org/koji/taskinfo?taskID=5295014 (f19-candidate)

Comment 92 Fedora Update System 2013-04-24 00:51:15 UTC
open-vm-tools-9.2.2-11.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/open-vm-tools-9.2.2-11.fc19

Comment 93 Simone Caronni 2013-04-24 08:18:37 UTC
Package Change Request
======================
Package Name: open-vm-tools
New Branches: el5 el6 f17 f18
Owners: slaanesh rjones ravindrakumar

Comment 94 Richard W.M. Jones 2013-04-24 08:22:59 UTC
http://autoqa.fedoraproject.org/report/rm4b

AutoQA is complaining about a dependency on /usr/sbin/ldconfig,
which I don't understand.  It looks like it's a false positive to me.

Comment 95 Simone Caronni 2013-04-24 08:26:46 UTC
(In reply to comment #94)
> http://autoqa.fedoraproject.org/report/rm4b
> 
> AutoQA is complaining about a dependency on /usr/sbin/ldconfig,
> which I don't understand.  It looks like it's a false positive to me.

As in comment #74, Ravindra used /usr/sbin/ldconfig instead of the guidelines' /sbin/ldconfig. I will push /sbin/ldconfig for all the other Fedora and RHEL branches.

(In reply to comment #89)
> I have removed the steps related to old versions of Fedora and RHEL. Please
> take a look at the final version of the SPEC file for Fedora 19 I would like
> to checkin:
> 
> SPEC File URL: https://www.dropbox.com/s/hzs0lk8l1reyaix/open-vm-tools.spec
> SRPM URL:
> https://www.dropbox.com/s/ilp61bz2ql64oa6/open-vm-tools-9.2.2-11.fc18.src.rpm

Sorry, I've not been able to connect and respond to the comment. I've asked for the commit acl in pkgdb and added the request for the additional branches.

In the spec file you can delete all the %defattr, Group and BuildRoot tags. Those are obsolete for Fedora. If you approve the acls I can remove the tags and rebuild.

Regards,
--Simone

Comment 96 Gwyn Ciesla 2013-04-24 12:07:28 UTC
Git done (by process-git-requests).

Comment 97 Simone Caronni 2013-04-24 14:17:58 UTC
I've built now the other branches, I've started from version 1 so upgrades to f19 will work even if I make some adjustment to the older branches.

I did apply the /sbin/ldconfig change and

ExclusiveArch:    %{ix86} x86_64

so it doesn't get built on ppc/ppc64 (RHEL/CentOS).

Comment 98 Gwyn Ciesla 2013-04-24 14:21:58 UTC
Unsetting flag.

Comment 99 Fedora Update System 2013-04-24 14:40:31 UTC
open-vm-tools-9.2.2-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/open-vm-tools-9.2.2-1.fc18

Comment 100 Fedora Update System 2013-04-24 14:42:11 UTC
open-vm-tools-9.2.2-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/open-vm-tools-9.2.2-1.fc17

Comment 101 Mark Mikofski 2013-04-24 16:26:47 UTC
FYI: a new stable open-vm-tools (open-vm-tools-9.2.3-1031360) has been released today 2013-04-24:

http://sourceforge.net/projects/open-vm-tools/files/open-vm-tools/stable-9.2.x/open-vm-tools-9.2.3-1031360.tar.gz

A pseudo list of fixes is on Dmitry Torokhov sf.net page:

https://sourceforge.net/users/dtor/

Comment 102 Fedora Update System 2013-04-24 16:39:52 UTC
open-vm-tools-9.2.2-11.fc19 has been pushed to the Fedora 19 testing repository.

Comment 103 Simone Caronni 2013-04-24 19:36:57 UTC
(In reply to comment #101)
> FYI: a new stable open-vm-tools (open-vm-tools-9.2.3-1031360) has been
> released today 2013-04-24:
> 
> http://sourceforge.net/projects/open-vm-tools/files/open-vm-tools/stable-9.2.
> x/open-vm-tools-9.2.3-1031360.tar.gz
> 
> A pseudo list of fixes is on Dmitry Torokhov sf.net page:
> 
> https://sourceforge.net/users/dtor/

Thanks for the info. I will wait on updating the Fedora 17 and 18 builds until Ravindra has updated the Fedora 19 one or granted me permissions on the branch.

In the tarball there's a Changelog and NEWS file with a list of the changes.

Comment 104 Ravindra Kumar 2013-04-24 20:48:33 UTC
> AutoQA is complaining about a dependency on /usr/sbin/ldconfig,
> which I don't understand.  It looks like it's a false positive to me.

I too thought so, because ldconfig comes from glibc package.

> In the spec file you can delete all the %defattr, Group and BuildRoot tags.
> Those are obsolete for Fedora. If you approve the acls I can remove the tags
> and rebuild.

Simone, I have addressed all these comments except 'Group' tag. It seems to be
optional, do you really want me to remove it? Here are the updated files:

SPEC File URL: https://www.dropbox.com/s/hxc3uh0hbzt54we/open-vm-tools.spec
SRPM URL: https://www.dropbox.com/s/h3rwukk85qc3eac/open-vm-tools-9.2.2-12.fc18.src.rpm

I will push the new source code after current push to Fedora 19 is successful.

Comment 105 Ravindra Kumar 2013-04-24 21:52:30 UTC
Simone, thanks for pushing it to f17 and f18. I have granted you commit
permissions on f19 and devel branches.

Thanks for the review, I have made the changes you have asked above.

Comment 106 Fedora Update System 2013-04-25 01:09:47 UTC
open-vm-tools-9.2.2-12.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/open-vm-tools-9.2.2-12.fc19

Comment 107 Ravindra Kumar 2013-04-25 17:42:11 UTC
I have modified the spec file to pick new upstream version. The new version has
fixed some things that no longer require few configure options, I have removed
those now.

Following are the updated files, I'm going to checkin. Please review.

SPEC File URL: https://www.dropbox.com/s/c1m1ez3btmj4y6j/open-vm-tools.spec
SRPM URL: https://www.dropbox.com/s/2gsya9bbex5xg9q/open-vm-tools-9.2.3-1.fc18.src.rpm

Comment 108 Fedora Update System 2013-04-25 19:21:12 UTC
open-vm-tools-9.2.3-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/open-vm-tools-9.2.3-1.fc19

Comment 109 Simone Caronni 2013-04-26 07:47:30 UTC
Sorry for the delay, I was quite busy. Spec file looks fine. Still has the Group tag which was considered optional [1] around Fedora 10 if I remember correctly.

I think we can stop using this bug for discussing packaging changes; please address me directly! :D

Thanks,
--Simone

[1] https://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag

Comment 110 Fedora Update System 2013-04-26 11:52:17 UTC
open-vm-tools-9.2.3-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/open-vm-tools-9.2.3-1.fc18

Comment 111 Fedora Update System 2013-04-26 11:52:45 UTC
open-vm-tools-9.2.3-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/open-vm-tools-9.2.3-1.fc17

Comment 112 Fedora Update System 2013-04-26 11:53:20 UTC
open-vm-tools-9.2.3-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/open-vm-tools-9.2.3-1.el6

Comment 113 Fedora Update System 2013-04-29 22:19:40 UTC
open-vm-tools-9.2.3-3.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/open-vm-tools-9.2.3-3.fc19

Comment 114 Fedora Update System 2013-05-02 07:22:45 UTC
open-vm-tools-9.2.3-4.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/open-vm-tools-9.2.3-4.fc19

Comment 115 Fedora Update System 2013-05-06 04:24:10 UTC
open-vm-tools-9.2.3-4.fc19 has been pushed to the Fedora 19 stable repository.

Comment 116 Fedora Update System 2013-05-06 19:44:30 UTC
open-vm-tools-9.2.3-5.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/open-vm-tools-9.2.3-5.fc19

Comment 117 Fedora Update System 2013-05-06 20:33:46 UTC
open-vm-tools-9.2.3-5.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/open-vm-tools-9.2.3-5.fc17

Comment 118 Fedora Update System 2013-05-06 20:34:11 UTC
open-vm-tools-9.2.3-5.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/open-vm-tools-9.2.3-5.fc18

Comment 119 Fedora Update System 2013-05-06 20:35:34 UTC
open-vm-tools-9.2.3-5.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/open-vm-tools-9.2.3-5.el6

Comment 120 Fedora Update System 2013-05-09 10:09:03 UTC
open-vm-tools-9.2.3-5.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 121 Fedora Update System 2013-05-09 10:11:09 UTC
open-vm-tools-9.2.3-5.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 122 Fedora Update System 2013-05-22 21:39:13 UTC
open-vm-tools-9.2.3-5.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 123 Richard W.M. Jones 2013-08-08 10:52:57 UTC
*** Bug 294321 has been marked as a duplicate of this bug. ***

Comment 124 Richard W.M. Jones 2013-08-08 10:53:02 UTC
*** Bug 789965 has been marked as a duplicate of this bug. ***


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