Bug 486390

Summary: Review Request: simspark - Spark physical simulation system
Product: [Fedora] Fedora Reporter: Hedayat Vatankhah <hedayatv>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, mtasaka, notting
Target Milestone: ---Flags: mtasaka: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 0.1-3.fc10 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-03-23 15:48:30 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 490254, 490975    
Bug Blocks:    

Description Hedayat Vatankhah 2009-02-19 15:50:07 UTC
Spec URL: http://hedayat.fedorapeople.org/simspark_review/0.1-1/simspark.spec
SRPM URL: http://hedayat.fedorapeople.org/simspark_review/0.1-1/simspark-0.1-1.fc10.src.rpm
Description:
Spark is a physical simulation system. The primary purpose of this system is
to provide a *generic* simulator for different kinds of simulations.
In these simulations, agents can participate as external processes.

Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=1139976

Rpmlint output: 
Doesn't report any errors for src.rpm and spec files. But for rpm packages:
simspark.x86_64: W: dangling-symlink /usr/share/simspark/fonts/VeraMono.ttf /usr/share/fonts/dejavu/DejaVuSansMono.ttf
simspark.x86_64: W: symlink-should-be-relative /usr/share/simspark/fonts/VeraMono.ttf /usr/share/fonts/dejavu/DejaVuSansMono.ttf
simspark-devel.x86_64: E: only-non-binary-in-usr-lib

Due to the new font packaging guidelines, this package doesn't contain any font files and it points to a font from dejavu-fonts-sans-mono package instead. And this package requires dejavu-fonts-sans-mono because of this.

Extra comments: 
In the latest version of rcssserver3d package (rcssserver3d-0.6 package is already in Fedora), this package is split into some sub-packages by upstream. Two main sub-packages are simspark and rcssserver3d. simspark package provides core libraries which were previously part of rcssserver3d package. But since simspark is a generic simulation framework and not soccer specific, it is provided as a separate package now.
So, the next version of rcssserver3d package (version 0.6.1 and later versions) will depend on this package.

A question:
Because of this separation, this package have a few files in /usr/bin which conflicts with rcssserver3d-0.6. (But rcssserver3d-0.6.1 won't have any conflicting files with simspark package). Therefore, users which simply update rcssserver3d will not face any problems, but if someone which has rcssserver3d-0.6 decides to install simspark, he can't do that since these packages have some conflicting files. 
Should I add a Conflicts: section to simspark spec file?!

Comment 1 Hedayat Vatankhah 2009-03-14 15:28:30 UTC
OK, I'll fix the issue with DevIL-1.7.8 with a patch. Please continue the review.

Comment 2 Mamoru TASAKA 2009-03-14 17:27:13 UTC
Some notes:

* BR
  - Would you explain where "BR: ImageMagick" is used?
  - "BR: gcc-c++" is redundant.

* Requires
  - Unfortunately, on F-11 dejavu-fonts-sans-mono is renamed
    to dejavu-sans-mono-fonts for some reason...

  - Is it what you expect that simspark main package always
    install ruby? I always have ruby installed on my system
    so I don't care about ruby dependency, however it does
    not seem that this package should always require ruby.

    Maybe splitting files depending on ruby into seperate
    subpackage is preferable.

* Build
  - As you already saw, this package won't build with DevIL
    1.7.8 (i.e. rawhide)

* Cflags
  - build.log shows some compilation information:
------------------------------------------------------------------
[  1%] Building CXX object utility/tinyxml/CMakeFiles/tinyxml_ex.dir/tinyxml.cpp.o
[  1%] Building C object utility/sfsexp/CMakeFiles/sexp.dir/io.c.o
------------------------------------------------------------------
    which is not useful. Please make build.log more verbose
    (ref:
    https://fedoraproject.org/wiki/Packaging/cmake )

  - And "make VERBOSE=1" actually shows Fedora specific compilation
    flags are not correctly honored, ref:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=1241115

  - Also, Fedora's default optimization level is -O2, while
    this package use -O3.

* ldconfig call
  - Calling /sbin/ldconfig on scriptlets is not needed for this
    package because no libraries are installed under default ldconfig
    search paths.
    ! By the way, please check if it is as you expect that no libraries
      are installed under default ldconfig search paths.


(In reply to comment #0)
> Should I add a Conflicts: section to simspark spec file?!  
  - I would write Explicit conflicts "Conflicts: rcssserver3d < 0.6.1",
    however other reviewers may say that it is not needed.

Comment 3 Hedayat Vatankhah 2009-03-16 13:14:49 UTC
Thank you for reviewing the package.
* BR:
 - Latex related cmake files look for "convert" utility of ImageMagic and will fail if it does not exist. I'm not sure if it is actually used during documentation generation.
 - OK, will be removed

* Requires:
 - Yes, each distro (F9, F10 and F11) use a different name! I'll either add conditional statements for each distro or a file level dependency. I think package dependency is preferred so I'll add that. (any suggestions?)

 - Yes. Ruby is highly integrated into simspark and is used to glue different plugins and subsystems together to create a functional part. So, I think there is no need to create subpackages for it. (to be more specific, zeitgeist library requires ruby, and other main libraries(oxygen and kerosin) require zeitgeist).

* Build: 
 - Fixed.

* Cflags:
 - sorry for that :( these issues will be fixed.

* ldconfig:
 - Yes, it is the default behavior of the upstream package. But, while most of the libraries are plugins, some of them are libraries which executable files will be linked to. Should I create a config file in /etc/ld.so.conf.d for /usr/lib{64}/simspark ?!

* OK, I'll add the conflicts statement.

Comment 4 Mamoru TASAKA 2009-03-16 18:08:56 UTC
(In reply to comment #3)
> Thank you for reviewing the package.
> * BR:
>  - Latex related cmake files look for "convert" utility of ImageMagic and will
> fail if it does not exist. I'm not sure if it is actually used during
> documentation generation.

- Then okay.

> * Requires:
>  - Yes, each distro (F9, F10 and F11) use a different name! I'll either add
> conditional statements for each distro or a file level dependency. I think
> package dependency is preferred so I'll add that. (any suggestions?)

- Package dependency is preferred than file dependency.

>  - Yes. Ruby is highly integrated into simspark and is used to glue different
> plugins and subsystems together to create a functional part. So, I think there
> is no need to create subpackages for it. (to be more specific, zeitgeist
> library requires ruby, and other main libraries(oxygen and kerosin) require
> zeitgeist).

- Thank you for explanation.

> * ldconfig:
>  - Yes, it is the default behavior of the upstream package. But, while most of
> the libraries are plugins, some of them are libraries which executable files
> will be linked to. Should I create a config file in /etc/ld.so.conf.d for
> /usr/lib{64}/simspark ?!

- Theoretically if the binaries trying to link against libraries 
  under %_libdir/simspark use rpath, there is no need to use
  ld.so.conf method. However if some of the libraries under 
  %_libdir/simspark are really _system wide_ libraries, I would
  suggest that such libraries should be moved under %_libdir.

Comment 6 Mamoru TASAKA 2009-03-17 18:55:07 UTC
Well, for -2:

* BR
  - build.log shows:
---------------------------------------------------------------
    99  -- Found the following Boost libraries:
   100  --   thread
   101  --   regex
   102  -- Could NOT find wxWidgets  (missing:  wxWidgets_FOUND)
---------------------------------------------------------------
    It seems that this can be enabled by BR: wxGTK-devel:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=1246607

* Path incorrectness
  - %_bindir/*-config (in -devel package) contains some wrong
    path information.
    e.g. /usr/bin/oxygen-config on i386:
---------------------------------------------------------------
    95   fi
    96   echo -L/usr//usr/lib/simspark $libs
    97  fi
   104   fi
   105   echo /usr//usr/lib/simspark/$convlib
   106  fi
---------------------------------------------------------------

Comment 7 Hedayat Vatankhah 2009-03-18 07:28:27 UTC
* BR:
Yes, that's intentional. Currently, wx related plugins are not used (they are used by a program named rsgedit, but it is still not ready for production). I also didn't include them in rcssserver3d-0.6 package. And since wxGTK dependency is not desired for people who don't need wx related plugins, I think I would create a subpackage for them. 
Anyway, if you think that it is better to include them now, I will create a sub-package for them.

* OK, will be fixed. Thanks.

Comment 9 Mamoru TASAKA 2009-03-18 09:19:03 UTC
(In reply to comment #7)
> * BR:
> Yes, that's intentional. Currently, wx related plugins are not used (they are
> used by a program named rsgedit, but it is still not ready for production). I
> also didn't include them in rcssserver3d-0.6 package. And since wxGTK
> dependency is not desired for people who don't need wx related plugins, I think
> I would create a subpackage for them. 

In such case, explicitly disabling wxGTK support is recommended.
Are there some option for cmake like "--disable-wx"??

Comment 10 Hedayat Vatankhah 2009-03-18 10:11:04 UTC
No, there is no such options currently. It just considers wxWidgets as an optional part, so it won't build wx plugins when it doesn't exists.
Do you think it is required?

Comment 11 Mamoru TASAKA 2009-03-18 17:25:49 UTC
Then, okay.

-------------------------------------------------------------
    This package (simspark) is APPROVED by mtasaka
-------------------------------------------------------------

Comment 12 Hedayat Vatankhah 2009-03-20 19:03:18 UTC
New Package CVS Request
=======================
Package Name: simspark
Short Description: Spark physical simulation system
Owners: hedayat
Branches: F-9 F-10

Comment 13 Kevin Fenzi 2009-03-22 05:39:27 UTC
cvs done.

Comment 14 Fedora Update System 2009-03-22 19:31:30 UTC
simspark-0.1-3.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/simspark-0.1-3.fc9

Comment 15 Fedora Update System 2009-03-22 19:31:36 UTC
simspark-0.1-3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/simspark-0.1-3.fc10

Comment 16 Mamoru TASAKA 2009-03-22 19:56:04 UTC
Can't F-10/9 rcssserver3d be upgraded so as not to get conflict
with simspark before simspark is pushed into F-10/9 repositories?

Otherwise after simspark is pushed into repositories
I am not sure if "$ yum -y install rcssserver3d" still works.

Comment 17 Hedayat Vatankhah 2009-03-23 10:56:52 UTC
new version of rcssserver3d requires simspark, so I wonder if it is possible to build the new version of rcssserver3d for F-10/9 before simspark appears in the repositories.
Anyway, there should be no problem installing the current version of rcssserver3d if the user don't install simspark.
BTW, I will update rcssserver3d today, so it should not be a big problem.

Comment 18 Mamoru TASAKA 2009-03-23 11:26:07 UTC
(In reply to comment #17)
> new version of rcssserver3d requires simspark, so I wonder if it is possible to
> build the new version of rcssserver3d for F-10/9 before simspark appears in the
> repositories.

Well, actually it is possible
- Ask rel-eng team (i.e. create a ticket on
  https://fedorahosted.org/rel-eng/ ) to get simspark tagged as
  dist-f{9,10}-override
  Example:
  https://fedorahosted.org/rel-eng/ticket/1065
- After tagging is done by rel-eng team, simspark is added to koji buildroot,
  however still it is not pushed into stable repository
- After rebuilding new rcssserver3d against simspark is done,
  you can submit a push request on bodhi for simspark and rcssserver3d
  as "one set".
- After simspark and new rcssserver3d are tagged as dist-f{9,10}-updates,
  you should ask rel-eng team to remove dist-f{9,10}-override tag.

Comment 19 Fedora Update System 2009-03-23 15:48:25 UTC
simspark-0.1-3.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 20 Fedora Update System 2009-03-23 15:52:59 UTC
simspark-0.1-3.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 21 Hedayat Vatankhah 2009-03-23 18:22:20 UTC
(In reply to comment #18)
> Well, actually it is possible
> - Ask rel-eng team (i.e. create a ticket on
>   https://fedorahosted.org/rel-eng/ ) to get simspark tagged as
>   dist-f{9,10}-override
>   Example:
>   https://fedorahosted.org/rel-eng/ticket/1065
> - After tagging is done by rel-eng team, simspark is added to koji buildroot,
>   however still it is not pushed into stable repository
> - After rebuilding new rcssserver3d against simspark is done,
>   you can submit a push request on bodhi for simspark and rcssserver3d
>   as "one set".
> - After simspark and new rcssserver3d are tagged as dist-f{9,10}-updates,
>   you should ask rel-eng team to remove dist-f{9,10}-override tag.  

Thank you very much. I should remember that the process could be more flexible sometimes! :) simspark is in the repositories now, but I will remember what you said for future. 
Thanks a lot