Bug 1418157 - Review Request: hypatia - A pure-C math library for 2D/3D graphics (matrix, vector, quaternion)
Summary: Review Request: hypatia - A pure-C math library for 2D/3D graphics (matrix, v...
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL: https://github.com/dagostinelli/hypatia
Whiteboard:
Depends On:
Blocks: FE-NEEDSPONSOR
TreeView+ depends on / blocked
 
Reported: 2017-02-01 03:37 UTC by Darryl T. Agostinelli
Modified: 2020-01-19 04:35 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-01-19 04:35:15 UTC
Type: Bug


Attachments (Terms of Use)

Description Darryl T. Agostinelli 2017-02-01 03:37:49 UTC
Spec Name or Url: https://github.com/dagostinelli/hypatia/releases/download/1.0.0/hypatia.spec
SRPM Name or Url:
https://github.com/dagostinelli/hypatia/releases/download/1.0.0/hypatia-1.0.0-1.fc24.src.rpm

This is my first package. I am in need of a sponsor for my package.  I am the upstream maintainer (original author) of hypatia.  

Upstream Link: https://github.com/dagostinelli/hypatia

Description:
Hypatia is a pure-C math library. It is almost 100% C89/C90 compliant. This library is intended for use in 2d/3d graphics program (such as games). Since it is not a general purpose math library, but a library meant for 3d graphics, certain opinions have been expressed in its design. One of those design choices, intended to help with speed, is that all objects (quaternions, matrices, vectors) are mutable. (That means that the objects change their values.) This was a purposeful design choice.

Help:
I wish I had a successful koji build, but I can't get past this error. Please advise:
`/var/tmp/rpm-tmp.Fklvv2: line 31: fg: no job control`

Koji Link:
https://koji.fedoraproject.org/koji/taskinfo?taskID=17535152

Comment 1 Michael Schwendt 2017-02-05 09:27:54 UTC
> Help:
> I wish I had a successful koji build, but I can't get past this error.
> Please advise:
> `/var/tmp/rpm-tmp.Fklvv2: line 31: fg: no job control`

Since the koji buildroots (using Mock) are more minimal, you're missing "BuildRequires: cmake".


> Summary:	A pure-C math library for 2D/3D graphics (matrix, vector, quaternion)

These leading articles are not only superfluous, they are really *really* ugly in almost every package tool that displays these summaries in a list.

  https://fedoraproject.org/wiki/Examples_of_good_package_summaries
  https://fedoraproject.org/wiki/Packaging:Guidelines#Summary_and_description

Also, I would advise against trying to squeeze details into the %summary instead of the %description. If it's a C library, it can be convenient to mention that in the summary, but whether it's "pure C" is a minor detail to expand on in the description. As one could argue about good summaries for a long time, I suggest:

  Summary: C library for mathematics in 2D/3D graphics


> Group:		System Environment/Libraries

https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections


> %package	devel
> Requires:	%{name} = %{version}-%{release}

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> This package contains the header files, static libraries and development

https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries


> %package	doc
> Summary:	Development documentation for %{name}
> Group:		Development/Libraries

If at all, it would be "Group: Documentation". The group tag is optional for a long time, however.


> Requires:	%{name}-devel = %{version}-%{release}

Certainly wrong. HTML documentation does not need the -devel package to be present. Please keep separate -doc packages free of dependencies. Unless the -doc files are in a special format and need the base program to display them.


> %description	doc
> This package contains the development documentation for %{name}.
> If you like to develop programs using %{name}-devel, you will
> need to install %{name}-doc.

This "you will need to" sounds like a "have to". Just describe the package contents and leave it to the reader whether to install the package or whether to copy files from it.

  %description	doc
  This package contains documentation for developing with %{n
ame}.

The %description of the -devel package is similar. Many -devel packages simply describe what they include and leave it to the reader whether to install the package for doing development themselves or building something someone else has developed:

  %description  devel
  This package contains the header files and library for developing
  with %{name}.


> %check
> ctest -V %{?_smp_mflags}

During rpmbuild, the %check section is executed _after_ %install, also because it may be necessary to test files installed into the %buildroot. Putting %check after %install in the spec file is the more logical choice.


> %clean
> rm -rf $RPM_BUILD_ROOT

Similarly here. The %clean section is executed last, and it should not be redefined unless you really need to override the default:

https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections


> %install
> rm -rf $RPM_BUILD_ROOT

https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections


> %files
> %defattr(-,root,root,0755)

https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions


> %doc README.md
> %{_libdir}/libhypatia*.so.*

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


> %dir %{_includedir}/hypatia
> %{_includedir}/hypatia/*.h
> %{_includedir}/hypatia/*.hpp

These three lines with wildcards could be replaced with just one line:

  %{_includedir}/hypatia/

Note that the trailing '/' is optional but increases readability as it suggests that this is a directory.


> %{_libdir}/libhypatia.a

https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries
 

> %files doc
> %defattr(-,root,root,0644)

Same as above. Also, the file permissions in this package are wrong. Run "rpmls -p hypatia-doc-1.0.0-1.fc25.noarch.rpm|grep ^d" to see.


> %dir %{_docdir}/%{name}
> %dir %{_docdir}/%{name}/html
> %{_docdir}/%{name}/html/*

Same as above with the header files. If you include entire directory trees, simply specify the directory itself. 

Do note, however, that you share %{_docdir}/%{name} with the base library package, and that may require more efforts, because it has not been trouble-free on older platforms when using the %doc macro.


> "config.h"
> #include "config.h"

It is inconvenient to rip this out of the "hypatia" namespace by expecting the compiler to find it directly in headers search path. Any program using its own "config.h" file (such as when using the GNU Autotools defaults) will have problems including the right one. Better have your API users do

  #include <hypatia/hypatia.h>

and be explicit about the "hypatia" directory.

Comment 2 Darryl T. Agostinelli 2017-02-05 16:01:45 UTC
Thank you for your review.  As this is my first time, I have some questions.

* After I make the revisions, shall I bump the version number in the .spec file or the release number or bump nothing?  

>Version:	1.0.0
>Release:	1%{dist}

vs

>Version:	1.0.0
>Release:	2%{dist}

vs

>Version:	1.0.1
>Release:	1%{dist}

* Is it preferred that there be a new .spec file or shall I replace the original .spec file with my revisions from your review?

* Regarding the Summary: A pure-C math library for 2D/3D graphics (matrix, vector, quaternion)

It is an important detail to mention that the library has to do with  matrices, vectors and quaternions so as to separate it from other graphical packages that deal with other aspects of graphics programming.  How about this:

C library for 2D/3D graphics (matrix, vector, quaternion)

* The library can be built from source with either 32-bit or 64-bit floats.  float vs double.  This is expressed with a build flag:

cmake . -DHYPATIA_SINGLE_PRECISION_FLOATS=ON

It is hard to say precisely what a user of the library would like to use.  I've defaulted it to double precision floats in the package in the assumption that its favorable over the single precision floats. In my own work, however, I prefer the single precision floats. What is your advice?

* While I refer to the library as 'Hypatia', as I'm making the corrections that you suggested, I noticed that the doc and license folder for other libraries have the lib prefix.  Is it more correct to cause the .spec file to label the various directories (doc, license, etc) as 'libhypatia' instead of 'hypatia'?

/usr/share/licenses/libhypatia
/usr/share/doc/libhypatia

vs

/usr/share/licenses/hypatia
/usr/share/doc/hypatia

Comment 3 Michael Schwendt 2017-02-20 22:41:26 UTC
> After I make the revisions, shall I bump the version number
> in the .spec file or the release number or bump nothing?  

Bumping plus a %changelog entry is preferred and good practice, too:
https://fedoraproject.org/wiki/FrequentlyMadePackagingMistakes


> Is it preferred that there be a new .spec file or shall I replace
> the original .spec file with my revisions from your review?

As you would want to include valid "Spec URL:" and "SRPM URL:" links in your review request, you may replace the older spec file at its original upload location, but a new "SRPM URL:" pointer will be needed in a bugzilla comment.

https://fedoraproject.org/wiki/Package_Review_Process

The fedora-review tool looks for both the "Spec URL:" and "SRPM URL:" lines, btw, so you could point it at this ticket and let it perform many helpful checks.


> How about this:
> 
> C library for 2D/3D graphics (matrix, vector, quaternion)

Good, too. In my opinion.


> It is hard to say precisely what a user of the library would like to use.
> I've defaulted it to double precision floats in the package in the
> assumption hat its favorable over the single precision floats. In my own
> work, however, I prefer the single precision floats. What is your advice?

That's a strange question. You are the author of the library. You prefer floats, but you assume that library users would prefer doubles. Are there known library API users? If so, ask them? Else offer what you consider the better, superior default. If that's too limiting, it may be necessary to offer the lib as two shared libs, and API users would need to include a specific header and link a specific lib, too.


> While I refer to the library as 'Hypatia', as I'm making the
> corrections that you suggested, I noticed that the doc and license
> folder for other libraries have the lib prefix.  Is it more correct
> to cause the .spec file to label the various directories (doc,
> license, etc) as 'libhypatia' instead of 'hypatia'?

Where the %doc and %license macros store their files depends on %name.

Fedora's package names are supposed to stay close to the upstream name. If your upstream name is "Hypatia" or "hypatia", there is no naming guideline to add a "lib" prefix at Fedora. Some other dists do that, but not Fedora. There are enough library package names that don't start with a "lib" prefix.

https://fedoraproject.org/wiki/Packaging:Guidelines#Naming
https://fedoraproject.org/wiki/Packaging:Naming?rd=Packaging:NamingGuidelines#General_Naming

Comment 4 Darryl T. Agostinelli 2020-01-19 04:35:15 UTC
For now, I'm rescinding this.


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