Bug 486390
Summary: | Review Request: simspark - Spark physical simulation system | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Hedayat Vatankhah <hedayatv> |
Component: | Package Review | Assignee: | Mamoru TASAKA <mtasaka> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | 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
OK, I'll fix the issue with DevIL-1.7.8 with a patch. Please continue the review. 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. 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. (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. New files: SPEC:http://hedayat.fedorapeople.org/simspark_review/0.1-2/simspark.spec SRPM:http://hedayat.fedorapeople.org/simspark_review/0.1-2/simspark-0.1-2.fc10.src.rpm 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 --------------------------------------------------------------- * 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. SRPM: http://hedayat.fedorapeople.org/simspark_review/0.1-3/simspark-0.1-3.fc10.src.rpm SPEC: http://hedayat.fedorapeople.org/simspark_review/0.1-3/simspark.spec (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"?? 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? Then, okay. ------------------------------------------------------------- This package (simspark) is APPROVED by mtasaka ------------------------------------------------------------- New Package CVS Request ======================= Package Name: simspark Short Description: Spark physical simulation system Owners: hedayat Branches: F-9 F-10 cvs done. simspark-0.1-3.fc9 has been submitted as an update for Fedora 9. http://admin.fedoraproject.org/updates/simspark-0.1-3.fc9 simspark-0.1-3.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/simspark-0.1-3.fc10 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. 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. (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. 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. 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. (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 |