[Bug 1338339] Review Request: openrave - Open Robotics Automation Virtual Environment

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1338339



--- Comment #7 from Till Hofmann <hofmann@xxxxxxxxxxxxxxxxxxx> ---
Spec URL: https://thofmann.fedorapeople.org/openrave.spec
SRPM URL:
https://thofmann.fedorapeople.org/openrave-0.9.0-15.git8bfb8a6.fc23.src.rpm
koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=14303392

Thank you for your comments!

(In reply to Igor Gnatenko from comment #5)
> ---
> main pkg
>  
> > Source0:        https://github.com/rdiankov/openrave/archive/%{commit}/openrave-%{commit}.tar.gz
> I would prefer to use:
> https://github.com/rdiankov/openrave/archive/%{commit}/openrave-
> %{shortcommit}.tar.gz
> Up to you.

Done.

> 
> > Provides:       bundled(ivcon) = 0
> don't need version here

done - but rpmlint gives a warning. Should I ignore it?
> 
> ---
> -devel subpkg
> 
> > Requires:       %{name} = %{version}-%{release}
> Forgot to add %{?_isa} after %{name}

Fixed.

> 
> ---
> %build section
> 
> >   -DCMAKE_BUILD_TYPE=RelWithDebInfo \
> not needed, %cmake does this for you
> 
> >   -DBASH_COMPLETION_DIR="%{_sysconfdir}/bash_completion.d" \
> I think it could be detected automatically (in CMakeLists, didn't check
> upstream sources). Anyway proper path is
> %{_datadir}/bash-completion/completions/

I fixed the path and removed the cmake flag.
> 
> > make %{?_smp_mflags}
> can be replaced with %make_build, up to you

Replaced.

> 
> ---
> %install section
> 
> > make install DESTDIR=%{buildroot}
> can be replaced with %make_install, up to you

Replaced.

> 
> > find %{buildroot} -name '*.la' -exec rm -f {} ';'
> replace with: find %{buildroot}%{_libdir} -type f -name '*.la' -delete ';'

Done.

> 
> > bash_completion stuff
> see above
> 
> ---
> %files sections
> 
> -> once you will move bash_completion stuff into %{_datadir} remove
> %config(noreplace)

Fixed

> 
> > %{_datadir}/%{name}/openrave.bash
> I think it shouldn't go here

I completely removed the file because it's not useful for a systemwide install.
The only thing it does is to set up PATH, PYTHONPATH etc. But for a system
install, everything is in the respective path anyway.

> 
> > %{_bindir}/openrave-createplugin.py
> I would prefer to have this file without .py suffix
> Also would be better to change shebang to something like /usr/bin/python2
> and add Requires: python2-%{name} because this script requires Python module

I replaced all shebangs and renamed the script to openrave-createplugin. I also
added the dependency.

> 
> ---
> Other notes
> 
> -> Is it possible to build python3 subpackage?

Unfortunately no, because flann is only available for python2 right now. I'll
add a python3 package when flann supports python3.

I did some additional changes:
* I added locale files
* I updated to a newer commit that included our patches
* I added a patch for some issues with relative paths. I'll send the patch
upstream soon.

As you suggested, I removed the date from the release. I *think* this is a
post-release, that's why I chose this release tag. I sent an email to upstream
to confirm this is indeed the case. If not, I'll update the Release tag
accordingly.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://lists.fedoraproject.org/admin/lists/package-review@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]