[Bug 1536852] Review Request: molsketch - Molecular Structures Editor

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

 



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



--- Comment #2 from Alexander Ploumistos <alex.ploumistos@xxxxxxxxx> ---
Hello Antonio and thanks for taking this review request.


(In reply to Antonio Trande from comment #1)
> - Are you sure that 'qca' (or qca-qt5?) is required for building?

Not really and neither was upstream. I commented it out, didn't notice any
problems. The OpenSUSE spec file also listed libqt5-qttools as a requirement
for the devel package, but we don't have that in Fedora. Do you know what that
is about?


> - Make arch-specific the 'openbabel' explicit request:
> Requires:  openbabel%{?_isa}

Changed it, but why is that needed?


> - devel subpackage requires the main package (that contains libmolsketch.so):
> Requires: %{name}%{?_isa} = %{version}-%{release}

Fixed.


> - Remove that macros in comments or use %% for disabling them.

Did not know about the % escaping, did that.


> - Use %make_install in the %install section

Fixed.


> - Remove INSTALL as documentation

Removed.


> - %{_libdir}/lib*.so* installs unversioned libraries too. Change it with
>   %{_libdir}/lib*.so.*

Fixed. Can you point me to any good resource regarding versioned-unversioned
libraries?


> - Regarding your question on devel mailing list, there could be a problem
> with some architectures (like s390x) about how to set correctly the
> libraries directory.
> In particular, 
> 
> obabeliface/obabeliface.pro
> libmolsketch/libmolsketch.pro
> 
> contain a "DESTDIR = ../lib" line. To fix try to add the option
> ""MSK_INSTALL_LIBS=%{_libdir}" to qmake's line.

Thanks, it builds now.


> - Set MSK_INSTALL_INCLUDES to %{_includedir}/lib%{name}

I also changed

%files devel
%{_includedir}/%{name}

to

%files devel
%{_includedir}/lib%{name}

because the build failed complaining about a missing file.


> - /usr/share/icons/hicolor/scalable/mimetypes must not be owned by this
> package.

I declared both images in the %files section, perhaps I should just have gone a
level below %{_datadir}/icons/hicolor/scalable/ and used the wildcard there.


> - doc is a stand-alone package, it must provide an own license file.

Added.


> - Most files are licensed under GPLv2+ license; you can use it as License
> file i guess.

Changed to GPLv2+.


Spec URL: https://alexpl.fedorapeople.org/packages/Molsketch/molsketch.spec
SRPM URL:
https://alexpl.fedorapeople.org/packages/Molsketch/molsketch-0.5.1-3.fc28.src.rpm
koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=24370202


There is one serious regression now. When the program starts, I can see that it
is not using OpenBabel, in the bottom right corner it shows everything with a
minus (OpenBabel, InChI, gen2d). Can you tell where I've messed up?

-- 
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
To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux