Bug 2283055 - Review Request: supernovas - The SuperNOVAS C/C++ astrometry library
Summary: Review Request: supernovas - The SuperNOVAS C/C++ astrometry library
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Mattia Verga
QA Contact: Fedora Extras Quality Assurance
URL: https://smithsonian.github.io/SuperNO...
Whiteboard:
: 2282872 2282874 2282877 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2024-05-23 19:38 UTC by Attila Kovacs
Modified: 2024-06-19 07:56 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2024-06-19 07:56:25 UTC
Type: ---
Embargoed:
mattia.verga: fedora-review+


Attachments (Terms of Use)
The .spec file difference from Copr build 7482753 to 7485576 (3.58 KB, patch)
2024-05-24 17:17 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 7492432 to 7492436 (2.79 KB, patch)
2024-05-25 23:57 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 7492436 to 7494765 (3.96 KB, patch)
2024-05-27 07:15 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 7494765 to 7496349 (5.60 KB, patch)
2024-05-27 19:05 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 7496349 to 7496545 (1.75 KB, patch)
2024-05-27 20:50 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 7496545 to 7528738 (6.12 KB, patch)
2024-06-04 14:33 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 7528738 to 7585895 (2.02 KB, patch)
2024-06-10 12:52 UTC, Fedora Review Service
no flags Details | Diff

Description Attila Kovacs 2024-05-23 19:38:24 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07482734-supernovas/supernovas.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07482734-supernovas/supernovas-1.0.1-1.fc41.src.rpm
Description: 

SuperNOVAS is a C/C++ astronomy library, providing high-precision 
astronomical calculations such as one might need for running an observatory or 
a precise planetarium program. It is a fork of the Naval Observatory Vector 
Astrometry Software (NOVAS) C version 3.1, providing bug fixes and making it 
easier to use overall.

The main goal of SuperNOVAS is to improve usability, add new features, promote 
best practices, and provide accessible documentation -- all while retaining 
100% API compatibility with NOVAS C 3.1. Thus, if you have written code for 
NOVAS C 3.1, it should readily work with SuperNOVAS also.

SuperNOVAS is entirely free to use without licensing restrictions. Its source 
code is compatible with the C90 standard, and hence should be suitable for old 
and new platforms alike. It is light-weight and easy to use, with full support 
for the IAU 2000/2006 standards for sub-micro-arc-second position calculations.

Fedora Account System Username: attipaci


This is my second attempt to package SuperNOVAS for Fedora. Previously, I had separate -libs, -devel, and -doc packages (.spec files). This was reviewed by Mattia Verga, who suggested to build them as sub-packages instead (and drop the -libs). So here it comes as such.


Since I have not published packages to Fedora before, I will be needing a sponsor to navigate me through the process. I look forward to the journey.


Thanks for your help in advance,

-- Attila Kovacs

Comment 1 Fedora Review Service 2024-05-23 19:46:58 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/7482753
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2283055-supernovas/fedora-rawhide-x86_64/07482753-supernovas/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 2 Attila Kovacs 2024-05-24 09:11:09 UTC
*** Bug 2282872 has been marked as a duplicate of this bug. ***

Comment 3 Attila Kovacs 2024-05-24 09:12:41 UTC
*** Bug 2282877 has been marked as a duplicate of this bug. ***

Comment 4 Attila Kovacs 2024-05-24 09:13:59 UTC
*** Bug 2282874 has been marked as a duplicate of this bug. ***

Comment 5 Mattia Verga 2024-05-24 15:20:45 UTC
A quick informal review:

> Group:			Development/Libraries
The Group: tag SHOULD NOT be used.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections

> %package doc
The -doc subpackage must require the main package, as it puts files under '%{_datadir}/%{name}' which is owned by the main package.

> %install
> rm -rf %{buildroot}
The contents of the buildroot SHOULD NOT be removed in the first line of %install.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections

> %files
Nothing owns %{_datadir}/%{name}
You should explicitly add '%dir %{_datadir}/%{name}'

> %doc %{_datadir}/%{name}/doc/*
I think it's better to use '%doc %{_datadir}/%{name}/doc' so that the subpackage will own both the directory and its content.

> %doc %{_mandir}/man3/*
Files installed in %{_mandir} are automatically marked by RPM as documentation. Thus it is not necessary to use %doc.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages

Comment 6 Attila Kovacs 2024-05-24 16:33:24 UTC
Thanks Mattia!

All easy fixes. I'm running the test build on Copr now. I'll submit the updated spec and SRPM once it builds successfully.

cheers,
-- Attila.

Comment 8 Fedora Review Service 2024-05-24 17:17:10 UTC
Created attachment 2034987 [details]
The .spec file difference from Copr build 7482753 to 7485576

Comment 9 Fedora Review Service 2024-05-24 17:17:12 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/7485576
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2283055-supernovas/fedora-rawhide-x86_64/07485576-supernovas/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 10 Attila Kovacs 2024-05-25 10:48:01 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07489435-supernovas/supernovas.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07489435-supernovas/supernovas-1.0.1-1.fc41.src.rpm

 - Fix `SONAME` versioning (to major only)
 - Some future proofing of file names following upstream changes intended to make packaging easier.

-- Attila.

Comment 11 Attila Kovacs 2024-05-25 13:45:38 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07490462-supernovas/supernovas.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07490462-supernovas/supernovas-1.0.1-1.fc41.src.rpm

 - Split out `cio-data` subpackage for large non-essential data component, resulting in a much smaller library footprint.

-- Attila.

Comment 12 Attila Kovacs 2024-05-25 19:50:22 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07491324-supernovas/supernovas.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07491324-supernovas/supernovas-1.0.1-1.fc41.src.rpm

 - Use `%{_docdir} macro to install docs in the right place
 - Omit manpages (these doxygen generated ones aren't very good, and the HTML docs are way better)
 - Link .so libs against -lm and also against -lsupernovas to resolve symbol dependencies as best as we can.

-- Attila

Comment 13 Fedora Review Service 2024-05-25 23:53:40 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/7492436
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2283055-supernovas/fedora-rawhide-x86_64/07492436-supernovas/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 14 Fedora Review Service 2024-05-25 23:57:22 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/7492430
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2283055-supernovas/fedora-rawhide-x86_64/07492430-supernovas/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 15 Fedora Review Service 2024-05-25 23:57:33 UTC
Created attachment 2035167 [details]
The .spec file difference from Copr build 7492432 to 7492436

Comment 16 Fedora Review Service 2024-05-25 23:57:35 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/7492432
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2283055-supernovas/fedora-rawhide-x86_64/07492432-supernovas/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 17 Attila Kovacs 2024-05-27 07:02:59 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07494754-supernovas/supernovas.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07494754-supernovas/supernovas-1.0.1-1.fc41.src.rpm

 - Added `%check` that runs regression testing
 - `cio-data` sub-package installs endian-dependent binary data into `%{_libdir}/supernovas/`. (The old `noarch` way was bad on LE platforms).


-- Attila.

Comment 18 Fedora Review Service 2024-05-27 07:15:07 UTC
Created attachment 2035277 [details]
The .spec file difference from Copr build 7492436 to 7494765

Comment 19 Fedora Review Service 2024-05-27 07:15:09 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/7494765
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2283055-supernovas/fedora-rawhide-x86_64/07494765-supernovas/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 20 Attila Kovacs 2024-05-27 18:55:46 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07496328-supernovas/supernovas.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07496328-supernovas/supernovas-1.0.1-1.fc41.src.rpm

 - Separate optional `solsys1` and `solsys2` legacy plugin subpackages.
 - Further examples in `devel` sub-package documentation.
 - Added regression testing under %check section.
 - Fixes for portability for non-Intel x86 platforms.

-- Attila

Comment 21 Fedora Review Service 2024-05-27 19:05:12 UTC
Created attachment 2035355 [details]
The .spec file difference from Copr build 7494765 to 7496349

Comment 22 Fedora Review Service 2024-05-27 19:05:15 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/7496349
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2283055-supernovas/fedora-rawhide-x86_64/07496349-supernovas/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 23 Attila Kovacs 2024-05-27 20:35:47 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07496481-supernovas/supernovas.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07496481-supernovas/supernovas-1.0.1-1.fc41.src.rpm

 - Add `%{_isa}` to the dependence of binary packages
 - Fix improper package tag
 - Edits to package descriptions

At this point, I think the package is ready for reviewing. 

Thanks in advance,

-- Attila

Comment 24 Fedora Review Service 2024-05-27 20:50:18 UTC
Created attachment 2035372 [details]
The .spec file difference from Copr build 7496349 to 7496545

Comment 25 Fedora Review Service 2024-05-27 20:50:21 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/7496545
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2283055-supernovas/fedora-rawhide-x86_64/07496545-supernovas/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 26 Mattia Verga 2024-05-30 09:46:59 UTC
Since you are already backporting a lot of fixes from upstream master branch by manually changing sources in the specfile, do you think it worth to package a snapshot of the master branch instead of the release tag?
See https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_snapshots and https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_commit_example

Or maybe the release tag + backporting the relevant patches (if they're few enough) instead of manually hacking the code in the spec file, which will improve readability.

Also note that 'Recommends: %{name}-cio-data = %{version}-%{release}' will trigger -cio-data installation by default. If you think it's not necessary for most use cases you might want to use 'Suggests:' (which effectively does nothing).

Comment 27 Attila Kovacs 2024-05-30 10:11:31 UTC
Thanks Mattia,


I agree with your assessment. The first package is a little special, because the current upstream release wasn't really designed with packaging in mind. This should be much improved with the next upstream release, scheduled for September 1... In the meantime, I think I'll create a branch from the current `main`, and roll-back the version in `novas.h` to 1.0.1-2 (from 1.0.2-devel). Then use that branch for the .spec, with the matching version number. This should be OK, since the current 'main' is still 100% ABI compatible with 1.0.1...

And, I will change 'Recommends' to 'Suggests' as you recommended, since the intention was to make the cio-data sub-package optional.

I'll be back with a new spec soon...

-- Attila.

Comment 28 Attila Kovacs 2024-05-30 11:18:27 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07507827-supernovas/supernovas.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07507827-supernovas/supernovas-1.0.1-2.fc41.src.rpm

 - Created new upstream tag `v1.0.1-2` (branched from main and rolled-back version number).
 - The %build section is now super simple.

-- Attila

Comment 29 Attila Kovacs 2024-05-30 13:35:14 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07508047-supernovas/supernovas.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07508047-supernovas/supernovas-1.0.1-2.fc41.src.rpm

 - With the upgrade to 1.0.1-2, there is no more need for `gfortran` build dep -- removed from spec.
 - `Recommends` was changed to `Suggests` (already in the previous .spec above).

-- Attila.

Comment 31 Fedora Review Service 2024-06-04 14:33:21 UTC
Created attachment 2036275 [details]
The .spec file difference from Copr build 7496545 to 7528738

Comment 32 Fedora Review Service 2024-06-04 14:33:24 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/7528738
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2283055-supernovas/fedora-rawhide-x86_64/07528738-supernovas/fedora-review/review.txt

Found issues:

- Upstream MD5sum check error, diff is in /var/lib/copr-rpmbuild/results/supernovas/diff.txt
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 33 Mattia Verga 2024-06-08 17:07:42 UTC
A few issues while reviewing the package:

- Sources in the SRPM does not match with those downloaded from the upstream URL

- No known owner of /usr/lib64/supernovas
Perhaps the -cio-data subpackage should just own the entire %{_libdir}/%{name} directory?

- Macros in: supernovas (description)
I think this error is due to the presence of a '%' character in the description. It should be escaped (I think using a double '%%')

- rpmlint errors
supernovas-solsys2.x86_64: E: undefined-non-weak-symbol /usr/lib64/libsolsys2.so.1.0.1 novas_trace	(/usr/lib64/libsolsys2.so.1.0.1)
supernovas-solsys2.x86_64: E: undefined-non-weak-symbol /usr/lib64/libsolsys2.so.1.0.1 jplint_	(/usr/lib64/libsolsys2.so.1.0.1)
supernovas-solsys2.x86_64: E: undefined-non-weak-symbol /usr/lib64/libsolsys2.so.1.0.1 novas_error	(/usr/lib64/libsolsys2.so.1.0.1)
supernovas-solsys2.x86_64: E: undefined-non-weak-symbol /usr/lib64/libsolsys2.so.1.0.1 jplihp_	(/usr/lib64/libsolsys2.so.1.0.1)

- rpmlint warning
supernovas-cio-data.x86_64: W: only-non-binary-in-usr-lib
Is the cio_ra.bin an executable? Or it's just a resource? If the latter, maybe it can be moved under %{_datadir}/%{name}?

Comment 34 Attila Kovacs 2024-06-08 21:22:29 UTC
Thanks Mattia,

I think that was my fault. I must have accidentally put in links from an older build. Sorry about that. I'll make sure to check on the above issues though before submitting a new spec and SRPM next...

-- A.

Comment 35 Attila Kovacs 2024-06-09 07:48:31 UTC

(In reply to Mattia Verga from comment #33)

Hi again Mattia,

Before I upload a new SPEC (and a new SRPM built from it on Copr), I wanted to get your opinions on the issues you brought up.

> A few issues while reviewing the package:
> 
> - Sources in the SRPM does not match with those downloaded from the upstream
> URL

This must have been me referencing the wrong Copr build, since the SRPM is built on Copr from the source, and so the only way they can diverge is if the build was an older one, from before the last change on upstream.

It does bring up a question of versioning though, which I wanted to clarify before proceeding. The upstream version is a tag on a branch that contains patches to make packaging easier, as per your earlier suggestion. It has an upstream version number '1.0.1-2'. As such, would the RPM version be '1.0.1' (and release '2'), or would it be '1.0.1-2' and the release whatever Fedora assigns, so the RPM may have a version number such as '1.0.1-2-1'? (I feel the latter is the more appropriate, but I'd like you to confirm).


> - No known owner of /usr/lib64/supernovas
> Perhaps the -cio-data subpackage should just own the entire
> %{_libdir}/%{name} directory?

Ye, that makes sense. I'll add a '%dir %{libdir}/%{name}' into %files for it...

> - Macros in: supernovas (description)
> I think this error is due to the presence of a '%' character in the
> description. It should be escaped (I think using a double '%%')

I think I removed these macros a while back (I don't see them in the spec any more). This is another indication that the Copr build I referenced last was an old one by mistake...

> - rpmlint errors
> supernovas-solsys2.x86_64: E: undefined-non-weak-symbol
> /usr/lib64/libsolsys2.so.1.0.1 novas_trace	(/usr/lib64/libsolsys2.so.1.0.1)
> supernovas-solsys2.x86_64: E: undefined-non-weak-symbol
> /usr/lib64/libsolsys2.so.1.0.1 jplint_	(/usr/lib64/libsolsys2.so.1.0.1)
> supernovas-solsys2.x86_64: E: undefined-non-weak-symbol
> /usr/lib64/libsolsys2.so.1.0.1 novas_error	(/usr/lib64/libsolsys2.so.1.0.1)
> supernovas-solsys2.x86_64: E: undefined-non-weak-symbol
> /usr/lib64/libsolsys2.so.1.0.1 jplihp_	(/usr/lib64/libsolsys2.so.1.0.1)

OK, this is a bit more involved...

The latest spec (and upstream branch from which the package is built) fixes 2 of the 4 errors. The `solsys2` library just needed a link flag against the parent `libsupernovas` to get two of the missing symbols defined.

However, the missing `jplint_` and `jplihp_` are an intended feature of the `solsys2` plugin. As the description of this sub-package implies this plugin requires user-supplied code to work, and these functions are exactly the ones that must be defined by the user when using the `libsolsys2` plugin library. This whole subpackages is aimed for supporting legacy code only, for people who have existing code that define those `jplint_` and `jplihp_` symbols, and want to compile their code with supernovas. I can think of 3 ways to address this:

 1. Let it be as such, if that's OK. (The Debian package has passed the initial reviews and is moving to testing with the same setup.)
 2. I can define weak dummy implementations of the `jplint_` and `jplihp` symbols that simply return an error. That would cure the rpmlint errors, but it would also hide the fact that the `solsys2` plugin really requires these symbols be defined by the user-code. This would probably also mean that I'd have to create a new upstream branch/tag (e.g. 1.0.1-3) to distinguish it from the one used by the Debian package.
 3. Drop providing the `solsys2` plugin as a library altogether, and supply the source code as documentation (examples) only. This is fine, but it requires a bit of extra work for people who want to link their existing (old) code with the `solsys2` functionality.

What would be your solution?

> - rpmlint warning
> supernovas-cio-data.x86_64: W: only-non-binary-in-usr-lib
> Is the cio_ra.bin an executable? Or it's just a resource? If the latter,
> maybe it can be moved under %{_datadir}/%{name}?

It is a resource but it is a platform-dependent one -- so it has an 'arch' dependence. %{_datadir} has no arch-dependence, but %{_libdir} does, which is why I thought this resource might fit there best. Or do you think it's better to put arch-dependent data into {%_datadir}, perhaps under a %{name}/%{_isa} directory instead?


I'll await your responses before submitting a new SPEC with the appropriate changes.

Thanks in advance,

-- Attila.

Comment 36 Mattia Verga 2024-06-09 08:20:56 UTC
(In reply to Attila Kovacs from comment #35)
> 
> (In reply to Mattia Verga from comment #33)
> 
> Hi again Mattia,
> 
> Before I upload a new SPEC (and a new SRPM built from it on Copr), I wanted
> to get your opinions on the issues you brought up.
> 
> > A few issues while reviewing the package:
> > 
> > - Sources in the SRPM does not match with those downloaded from the upstream
> > URL
> 
> This must have been me referencing the wrong Copr build, since the SRPM is
> built on Copr from the source, and so the only way they can diverge is if
> the build was an older one, from before the last change on upstream.
> 
> It does bring up a question of versioning though, which I wanted to clarify
> before proceeding. The upstream version is a tag on a branch that contains
> patches to make packaging easier, as per your earlier suggestion. It has an
> upstream version number '1.0.1-2'. As such, would the RPM version be '1.0.1'
> (and release '2'), or would it be '1.0.1-2' and the release whatever Fedora
> assigns, so the RPM may have a version number such as '1.0.1-2-1'? (I feel
> the latter is the more appropriate, but I'd like you to confirm).
> 

'1.0.1-2-1' would not be a valid NVR. You could however do '1.0.1.2-1' (1.0.1.2 version and 1 release) if you think it doesn't worth to release a '1.0.2' bugfix version.

You can check the diff in the automatic COPR build in comment#32.

> 
> > - No known owner of /usr/lib64/supernovas
> > Perhaps the -cio-data subpackage should just own the entire
> > %{_libdir}/%{name} directory?
> 
> Ye, that makes sense. I'll add a '%dir %{libdir}/%{name}' into %files for
> it...
> 
> > - Macros in: supernovas (description)
> > I think this error is due to the presence of a '%' character in the
> > description. It should be escaped (I think using a double '%%')
> 
> I think I removed these macros a while back (I don't see them in the spec
> any more). This is another indication that the Copr build I referenced last
> was an old one by mistake...

I think in the "100% API compatibility" phrase RPM doesn't like that '%'...

> 
> > - rpmlint errors
> > supernovas-solsys2.x86_64: E: undefined-non-weak-symbol
> > /usr/lib64/libsolsys2.so.1.0.1 novas_trace	(/usr/lib64/libsolsys2.so.1.0.1)
> > supernovas-solsys2.x86_64: E: undefined-non-weak-symbol
> > /usr/lib64/libsolsys2.so.1.0.1 jplint_	(/usr/lib64/libsolsys2.so.1.0.1)
> > supernovas-solsys2.x86_64: E: undefined-non-weak-symbol
> > /usr/lib64/libsolsys2.so.1.0.1 novas_error	(/usr/lib64/libsolsys2.so.1.0.1)
> > supernovas-solsys2.x86_64: E: undefined-non-weak-symbol
> > /usr/lib64/libsolsys2.so.1.0.1 jplihp_	(/usr/lib64/libsolsys2.so.1.0.1)
> 
> OK, this is a bit more involved...
> 
> The latest spec (and upstream branch from which the package is built) fixes
> 2 of the 4 errors. The `solsys2` library just needed a link flag against the
> parent `libsupernovas` to get two of the missing symbols defined.
> 
> However, the missing `jplint_` and `jplihp_` are an intended feature of the
> `solsys2` plugin. As the description of this sub-package implies this plugin
> requires user-supplied code to work, and these functions are exactly the
> ones that must be defined by the user when using the `libsolsys2` plugin
> library. This whole subpackages is aimed for supporting legacy code only,
> for people who have existing code that define those `jplint_` and `jplihp_`
> symbols, and want to compile their code with supernovas. I can think of 3
> ways to address this:
> 
>  1. Let it be as such, if that's OK. (The Debian package has passed the
> initial reviews and is moving to testing with the same setup.)
>  2. I can define weak dummy implementations of the `jplint_` and `jplihp`
> symbols that simply return an error. That would cure the rpmlint errors, but
> it would also hide the fact that the `solsys2` plugin really requires these
> symbols be defined by the user-code. This would probably also mean that I'd
> have to create a new upstream branch/tag (e.g. 1.0.1-3) to distinguish it
> from the one used by the Debian package.
>  3. Drop providing the `solsys2` plugin as a library altogether, and supply
> the source code as documentation (examples) only. This is fine, but it
> requires a bit of extra work for people who want to link their existing
> (old) code with the `solsys2` functionality.
> 
> What would be your solution?

I think that if that is expected, it should be fine. But I don't know much about C programming/linking, so I'll ask for some advice by more skilled users on how to deal those errors.

> 
> > - rpmlint warning
> > supernovas-cio-data.x86_64: W: only-non-binary-in-usr-lib
> > Is the cio_ra.bin an executable? Or it's just a resource? If the latter,
> > maybe it can be moved under %{_datadir}/%{name}?
> 
> It is a resource but it is a platform-dependent one -- so it has an 'arch'
> dependence. %{_datadir} has no arch-dependence, but %{_libdir} does, which
> is why I thought this resource might fit there best. Or do you think it's
> better to put arch-dependent data into {%_datadir}, perhaps under a
> %{name}/%{_isa} directory instead?
> 

mmm I'll ask for that too, I don't think I've never encountered such a situation...

> 
> I'll await your responses before submitting a new SPEC with the appropriate
> changes.
> 
> Thanks in advance,
> 
> -- Attila.

I'll let you know ASAP.
Also note that, while I can do a full review of the package, you will still need someone to sponsor you as described at https://docs.fedoraproject.org/en-US/fesco/Packager_sponsor_policy/

Comment 37 Attila Kovacs 2024-06-09 08:26:55 UTC
(In reply to Mattia Verga from comment #36)

Thanks Mattia for the quick reply,

I'll wait for your next update when you find out more on these 'special' cases... :-)

-- A.

Comment 38 Daniel Berrangé 2024-06-10 10:13:18 UTC
> > > - rpmlint warning
> > > supernovas-cio-data.x86_64: W: only-non-binary-in-usr-lib
> > > Is the cio_ra.bin an executable? Or it's just a resource? If the latter,
> > > maybe it can be moved under %{_datadir}/%{name}?
> > 
> > It is a resource but it is a platform-dependent one -- so it has an 'arch'
> > dependence. %{_datadir} has no arch-dependence, but %{_libdir} does, which
> > is why I thought this resource might fit there best. Or do you think it's
> > better to put arch-dependent data into {%_datadir}, perhaps under a
> > %{name}/%{_isa} directory instead?
> > 
>
> mmm I'll ask for that too, I don't think I've never encountered such a situation...

What Attila has done is fine. If the data resource is built in an architecture dependent format, then using %{_libdir}/%{name}/ is a suitable approach to take. This is just a warning in rpmlint, rather than an error, as a way to remind people to double-check suitability for %{_libdir} special cases.

Comment 39 Daniel Berrangé 2024-06-10 10:34:41 UTC
> > It does bring up a question of versioning though, which I wanted to clarify
> > before proceeding. The upstream version is a tag on a branch that contains
> > patches to make packaging easier, as per your earlier suggestion. It has an
> > upstream version number '1.0.1-2'. As such, would the RPM version be '1.0.1'
> > (and release '2'), or would it be '1.0.1-2' and the release whatever Fedora
> > assigns, so the RPM may have a version number such as '1.0.1-2-1'? (I feel
> > the latter is the more appropriate, but I'd like you to confirm).
> > 
>
> '1.0.1-2-1' would not be a valid NVR. You could however do '1.0.1.2-1' (1.0.1.2 version and 1 release) if you think it doesn't worth to release a '1.0.2' bugfix version.

What this special "1.0.1-2" tag has done is to effectively create a different versioning scheme for bug fix releases (4 digits) vs normal releases (3 digits). Valid, but unusual and undesirable IMHO.

If this "1.0.1-2" tag was a one-off very special edge case, I'd be inclined to pretend that this tag doesn't exist, and just use the original '1.0.1' version in the RPM as the latest .spec has done, on the basis that the RPM will update to 1.0.2 eventually making the special case disappear for Fedora.

If upstream /wants/ the ability in future to create bug-fix releases on stable branches, then I'd suggest to consider changing the upstream version numbering practices. Have the normal releases bump the middle digit (1.1.0, 1.2.0, 1.3.0, 1.4.0, etc), and leave the minor digit as '0', reserved in case you need a bug fix release (1.1.1, 1.1.2, 1.1.3, etc) on a branch. If this is desired, it isn't a blocker for adding the Fedora RPM, and again we ca just use '1.0.1' as the version in the RPM for now.


> >  1. Let it be as such, if that's OK. (The Debian package has passed the
> > initial reviews and is moving to testing with the same setup.)
> >  2. I can define weak dummy implementations of the `jplint_` and `jplihp`
> > symbols that simply return an error. That would cure the rpmlint errors, but
> > it would also hide the fact that the `solsys2` plugin really requires these
> > symbols be defined by the user-code. This would probably also mean that I'd
> > have to create a new upstream branch/tag (e.g. 1.0.1-3) to distinguish it
> > from the one used by the Debian package.
> >  3. Drop providing the `solsys2` plugin as a library altogether, and supply
> > the source code as documentation (examples) only. This is fine, but it
> > requires a bit of extra work for people who want to link their existing
> > (old) code with the `solsys2` functionality.
> >
> > What would be your solution?
>
> I think that if that is expected, it should be fine. But I don't know much about C programming/linking, so I'll ask for some advice by more skilled users on how to deal those errors.

I think (1) is acceptable. This is certainly unusual / not best practice, but if this is the way the library has been *intentionally* designed to work, then it is not for Fedora to tell upstream to re-write their code.

Comment 40 Daniel Berrangé 2024-06-10 10:41:33 UTC
> %build
>
> # Define where the library will look for the CIO locator data
> CIO_LOCATOR_FILE=%{_datadir}/%{name}/cio_ra.bin

You installed the cio_ra.bin to %{_libdir}, so I would presume this CIO_LOCATOR_FILE path needs to match that, as IIUC, this env variable is used to set the path compiled into the resulting library.

Comment 41 Daniel Berrangé 2024-06-10 10:53:06 UTC
> Also note that, while I can do a full review of the package, you will still need someone to sponsor you as described at https://docs.fedoraproject.org/en-US/fesco/Packager_sponsor_policy/

I have sponsored Attila now

Comment 42 Daniel Berrangé 2024-06-10 10:59:15 UTC
(In reply to Daniel Berrangé from comment #40)
> > %build
> >
> > # Define where the library will look for the CIO locator data
> > CIO_LOCATOR_FILE=%{_datadir}/%{name}/cio_ra.bin
> 
> You installed the cio_ra.bin to %{_libdir}, so I would presume this
> CIO_LOCATOR_FILE path needs to match that, as IIUC, this env variable is
> used to set the path compiled into the resulting library.

Actually in the build log we can see this env var is currently having no effect:

  + CIO_LOCATOR_FILE=/usr/share/supernovas/cio_ra.bin
  + make -j20 distro
  WARNING! No default CIO_LOCATOR_FILE defined.
  .        Will use local 'cio_ra.bin' if present.

I think you need to pass it as a make variable instead to have it honoured: eg

  make %{?_smp_mflags} distro CIO_LOCATOR_FILE=%{_datadir}/%{name}/cio_ra.bin

Comment 43 Attila Kovacs 2024-06-10 12:10:32 UTC
(In reply to Daniel Berrangé from comment #42)

Thanks Daniel -- both for sponsoring and for the good comments,

Yes, the 1.0.1-2 tag is a bit special (created to make it more friendly for packaging) -- but API-wise it's still just 1.0.1 really, so I'll keep it as such in the .spec. It will be all straightened out in the next upstream release in due in September. (It may be 1.0.2 or 1.1.0 -- I'll see depending on what goes into it...)

I have added the CIO_LOCATOR_FILE to the make command line, as you suggested. Good catch, thanks!

I'm now running a test build on Copr with the latest spec. If it looks good, I'll submit the spec and SRPM links below...

cheers,

-- A.

Comment 44 Attila Kovacs 2024-06-10 12:39:13 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07585819-supernovas/supernovas.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07585819-supernovas/supernovas-1.0.1-3.fc41.src.rpm

 - Set `CIO_LOCATOR_FILE` on the `make` command line.
 - 'cio-file' sub-package now owns %{libdir}/%{name}.
 - main package suggests `cio-file`, `solsys1` and `solsys2` sub-packages.
 - Package release number bumped to 3 to clearly distinguish from prior builds.

-- A.

Comment 45 Fedora Review Service 2024-06-10 12:52:21 UTC
Created attachment 2036924 [details]
The .spec file difference from Copr build 7528738 to 7585895

Comment 46 Fedora Review Service 2024-06-10 12:52:23 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/7585895
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2283055-supernovas/fedora-rawhide-x86_64/07585895-supernovas/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 47 Daniel Berrangé 2024-06-10 13:44:32 UTC
Not a must have requirement, but I usually suggest to minimize use wildcards at the top levels of generic directories. eg in the -devel package you have these two:

  %{_prefix}/include/*
  %{_libdir}/*.so

You don't have a very big list of files to add without a wildcard, so the wildcard isn't a huge benefit. Manually listed the expected files ensures that if there is some unexpected change or build system mistake in a future version, that causes headers/libraries to go missing, you'll see it quickly. I've seen too many cases over the years where files silently go missing and aren't noticed by the package maintainer due to wildcard usage.

These wildcards are fine to keep as-is though:

  %{_libdir}/lib%{name}.so.1{,.*}

>  - Set `CIO_LOCATOR_FILE` on the `make` command line.

Copr build logs show this now being honoured in the compiler args, so that's good.

Comment 48 Attila Kovacs 2024-06-10 15:24:32 UTC
Thanks again Daniel,

I'll take your advice on the wildcards for the next update. The next upstream release will anyway require a few small tweaks to move away from the (bit) special treatment of this one, so I'll just add that to it.

The whole platform dependent data also got me thinking... It's a relic for back compatibility with the original NOVAS C library, but it would be much nicer if that data was in a platform independent format. I guess, with a little work, I can change the code such that it can  process both the legacy (platform-dependent data), and a new format that is platform independent (with some magic bytes at the head to identify the format). Then, in the future, the CIO locator data can be a `noarch`, and a lot of the related headaches will go away with it. I'll probably try to do that for the September upstream release.

Also, I was thinking that I can give you write access to the .spec repo also (Smithsonian/supernovas-rpm-spec), in case you want to make tweaks yourself. Let me know.

cheers,

-- A.

Comment 49 Daniel Berrangé 2024-06-10 17:05:34 UTC
> Also, I was thinking that I can give you write access to the .spec repo also (Smithsonian/supernovas-rpm-spec), in case you want to make tweaks yourself. Let me know.

Thanks for the offer, but I'm fine just submitting pull requests if ever needed - its good practice to always have 2nd pair of eyes on changes before hitting git main.

Comment 50 Attila Kovacs 2024-06-10 19:42:41 UTC
Yes, that's better practice, I agree :-).

Comment 51 Mattia Verga 2024-06-16 07:11:25 UTC
Sorry for the delay in the review. Here are the results, package APPROVED:

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig not called in %post and %postun for Fedora 28 and later.
[x]: Package does not contain any libtool archives (.la)
[x]: Package contains no static executables.
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated", "*No copyright* The Unlicense". 146
     files have unknown license. Detailed output of licensecheck in
     /review/2283055-supernovas/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[x]: Macros in Summary, %description expandable at SRPM build time.
     Note: Macros in: supernovas (description)
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 94483 bytes in 6 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: The License field must be a valid SPDX expression.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in
     supernovas-solsys1 , supernovas-solsys2 , supernovas-cio-data
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: No rpmlint messages.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: supernovas-1.0.1-3.fc41.x86_64.rpm
          supernovas-solsys1-1.0.1-3.fc41.x86_64.rpm
          supernovas-solsys2-1.0.1-3.fc41.x86_64.rpm
          supernovas-cio-data-1.0.1-3.fc41.x86_64.rpm
          supernovas-devel-1.0.1-3.fc41.x86_64.rpm
          supernovas-doc-1.0.1-3.fc41.noarch.rpm
          supernovas-debuginfo-1.0.1-3.fc41.x86_64.rpm
          supernovas-debugsource-1.0.1-3.fc41.x86_64.rpm
          supernovas-1.0.1-3.fc41.src.rpm
================================== rpmlint session starts ==================================
rpmlint: 2.5.0
configuration:
    /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml
    /etc/xdg/rpmlint/fedora-legacy-licenses.toml
    /etc/xdg/rpmlint/fedora-spdx-licenses.toml
    /etc/xdg/rpmlint/fedora.toml
    /etc/xdg/rpmlint/scoring.toml
    /etc/xdg/rpmlint/users-groups.toml
    /etc/xdg/rpmlint/warn-on-functions.toml
rpmlintrc: [PosixPath('/tmp/tmptdnczjhq')]
checks: 32, packages: 9

supernovas-cio-data.x86_64: W: only-non-binary-in-usr-lib
supernovas-cio-data.x86_64: W: no-documentation
supernovas-solsys1.x86_64: W: no-documentation
supernovas-solsys2.x86_64: W: no-documentation
supernovas-doc.noarch: W: files-duplicate /usr/share/doc/supernovas/html/search/defines_8.js /usr/share/doc/supernovas/html/search/all_1a.js
supernovas-doc.noarch: W: files-duplicate /usr/share/doc/supernovas/html/search/variables_a.js /usr/share/doc/supernovas/html/search/all_e.js
 9 packages and 0 specfiles checked; 0 errors, 6 warnings, 61 filtered, 0 badness; has taken 0.8 s 




Rpmlint (debuginfo)
-------------------
Checking: supernovas-solsys1-debuginfo-1.0.1-3.fc41.x86_64.rpm
          supernovas-solsys2-debuginfo-1.0.1-3.fc41.x86_64.rpm
          supernovas-debuginfo-1.0.1-3.fc41.x86_64.rpm
================================== rpmlint session starts ==================================
rpmlint: 2.5.0
configuration:
    /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml
    /etc/xdg/rpmlint/fedora-legacy-licenses.toml
    /etc/xdg/rpmlint/fedora-spdx-licenses.toml
    /etc/xdg/rpmlint/fedora.toml
    /etc/xdg/rpmlint/scoring.toml
    /etc/xdg/rpmlint/users-groups.toml
    /etc/xdg/rpmlint/warn-on-functions.toml
rpmlintrc: [PosixPath('/tmp/tmp8_d9sj_v')]
checks: 32, packages: 3

 3 packages and 0 specfiles checked; 0 errors, 0 warnings, 22 filtered, 0 badness; has taken 0.3 s 





Rpmlint (installed packages)
----------------------------
============================ rpmlint session starts ============================
rpmlint: 2.5.0
configuration:
    /usr/lib/python3.13/site-packages/rpmlint/configdefaults.toml
    /etc/xdg/rpmlint/fedora-legacy-licenses.toml
    /etc/xdg/rpmlint/fedora-spdx-licenses.toml
    /etc/xdg/rpmlint/fedora.toml
    /etc/xdg/rpmlint/scoring.toml
    /etc/xdg/rpmlint/users-groups.toml
    /etc/xdg/rpmlint/warn-on-functions.toml
checks: 32, packages: 10

supernovas-solsys2.x86_64: E: undefined-non-weak-symbol /usr/lib64/libsolsys2.so.1.0.1 jplint_	(/usr/lib64/libsolsys2.so.1.0.1)
supernovas-solsys2.x86_64: E: undefined-non-weak-symbol /usr/lib64/libsolsys2.so.1.0.1 jplihp_	(/usr/lib64/libsolsys2.so.1.0.1)
supernovas-cio-data.x86_64: W: only-non-binary-in-usr-lib
supernovas-solsys1.x86_64: W: no-documentation
supernovas-solsys2.x86_64: W: no-documentation
supernovas-cio-data.x86_64: W: no-documentation
supernovas-doc.noarch: W: files-duplicate /usr/share/doc/supernovas/html/search/defines_8.js /usr/share/doc/supernovas/html/search/all_1a.js
supernovas-doc.noarch: W: files-duplicate /usr/share/doc/supernovas/html/search/variables_a.js /usr/share/doc/supernovas/html/search/all_e.js
 10 packages and 0 specfiles checked; 2 errors, 6 warnings, 72 filtered, 2 badness; has taken 1.1 s 



Source checksums
----------------
https://github.com/Smithsonian/SuperNOVAS/archive/refs/tags/v1.0.1-2.tar.gz :
  CHECKSUM(SHA256) this package     : bae54a263deef9d38083b3affa471de966ca87298cfc306e279ab337a7d4d5be
  CHECKSUM(SHA256) upstream package : bae54a263deef9d38083b3affa471de966ca87298cfc306e279ab337a7d4d5be


Requires
--------
supernovas (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libm.so.6()(64bit)
    rtld(GNU_HASH)

supernovas-solsys1 (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libm.so.6()(64bit)
    libsupernovas.so.1()(64bit)
    rtld(GNU_HASH)
    supernovas(x86-64)

supernovas-solsys2 (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libsupernovas.so.1()(64bit)
    rtld(GNU_HASH)
    supernovas(x86-64)

supernovas-cio-data (rpmlib, GLIBC filtered):
    supernovas(x86-64)

supernovas-devel (rpmlib, GLIBC filtered):
    libsolsys1.so.1()(64bit)
    libsolsys2.so.1()(64bit)
    libsupernovas.so.1()(64bit)
    supernovas(x86-64)
    supernovas-solsys1(x86-64)
    supernovas-solsys2(x86-64)

supernovas-doc (rpmlib, GLIBC filtered):
    supernovas

supernovas-debuginfo (rpmlib, GLIBC filtered):

supernovas-debugsource (rpmlib, GLIBC filtered):



Provides
--------
supernovas:
    libsupernovas.so.1()(64bit)
    supernovas
    supernovas(x86-64)

supernovas-solsys1:
    libsolsys1.so.1()(64bit)
    supernovas-solsys1
    supernovas-solsys1(x86-64)

supernovas-solsys2:
    libsolsys2.so.1()(64bit)
    supernovas-solsys2
    supernovas-solsys2(x86-64)

supernovas-cio-data:
    supernovas-cio-data
    supernovas-cio-data(x86-64)

supernovas-devel:
    supernovas-devel
    supernovas-devel(x86-64)

supernovas-doc:
    supernovas-doc

supernovas-debuginfo:
    debuginfo(build-id)
    libsupernovas.so.1.0.1-1.0.1-3.fc41.x86_64.debug()(64bit)
    supernovas-debuginfo
    supernovas-debuginfo(x86-64)

supernovas-debugsource:
    supernovas-debugsource
    supernovas-debugsource(x86-64)



Generated by fedora-review 0.10.0 (e79b66b) last change: 2023-07-24
Command line :/usr/bin/fedora-review -b 2283055
Buildroot used: fedora-rawhide-x86_64
Active plugins: Shell-api, C/C++, Generic
Disabled plugins: PHP, Perl, Haskell, Java, fonts, R, Ocaml, Python, SugarActivity
Disabled flags: EXARCH, EPEL6, EPEL7, DISTTAG, BATCH

Comment 52 Fedora Review Service 2024-06-16 07:11:45 UTC
Hello @attipaci,
since this is your first Fedora package, you need to get sponsored by a package
sponsor before it can be accepted.

A sponsor is an experienced package maintainer who will guide you through
the processes that you will follow and the tools that you will use as a future
maintainer. A sponsor will also be there to answer your questions related to
packaging.

You can find all active sponsors here:
https://docs.pagure.org/fedora-sponsors/

I created a sponsorship request for you:
https://pagure.io/packager-sponsors/issue/663
Please take a look and make sure the information is correct.

Thank you, and best of luck on your packaging journey.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

Comment 53 Mattia Verga 2024-06-16 07:16:04 UTC
You can now proceed with https://docs.fedoraproject.org/en-US/package-maintainers/New_Package_Process_for_New_Contributors/#import_commit_and_build_your_package

Note that I was wrong with trying to escape a '%' character in the description with a double '%%'... maybe the correct way to make it work is using:
%global _description %{expand:
Some text with %.}
You might want to adjust it before building the package.

Question not related to this review: can SuperNOVAS be used as a replacement for the old, unmaintained libnovas? If so, you might want to announce your package to INDI and Kstars upstream, as they're the only packages in Fedora using libnovas, which latest version is over 10 years ago. Maybe it's time to replace it.

Comment 54 Attila Kovacs 2024-06-16 07:46:29 UTC
Hi Mattia,


> Note that I was wrong with trying to escape a '%' character in the
> description with a double '%%'... maybe the correct way to make it work is
> using:
> %global _description %{expand:
> Some text with %.}

I think the simplest may be to spell-out 'percent', and not worry about escaping...

> You might want to adjust it before building the package.

I'll update the spec later today.

> Question not related to this review: can SuperNOVAS be used as a replacement
> for the old, unmaintained libnovas? If so, you might want to announce your
> package to INDI and Kstars upstream, as they're the only packages in Fedora
> using libnovas, which latest version is over 10 years ago. Maybe it's time
> to replace it.

I cannot seem to find any information on `libnovas` (maybe Google is failing me). I did find a different `libnova` package though with an upstream from https://sourceforge.net/projects/libnova/. SuperNOVAS is not related to that, however. 

If there is a different `libnovas`, can you send me a link that describes it, and what files it has (and from what upstream source)? I can then give a definitive answer on that...

-- A.

Comment 55 Mattia Verga 2024-06-16 07:49:58 UTC
(In reply to Attila Kovacs from comment #54)
> 
> I cannot seem to find any information on `libnovas` (maybe Google is failing
> me). I did find a different `libnova` package though with an upstream from
> https://sourceforge.net/projects/libnova/. SuperNOVAS is not related to
> that, however. 
> 
> If there is a different `libnovas`, can you send me a link that describes
> it, and what files it has (and from what upstream source)? I can then give a
> definitive answer on that...
> 
> -- A.

yes, I mispelled, I meant libnova of course... thanks

Comment 57 Mattia Verga 2024-06-18 07:23:30 UTC
(In reply to Attila Kovacs from comment #56)
> Spec URL:
> https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/
> fedora-rawhide-x86_64/07619945-supernovas/supernovas.spec
> SRPM URL:
> https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/
> fedora-rawhide-x86_64/07619945-supernovas/supernovas-1.0.1-3.fc41.src.rpm
> 
>  - Spelled out 'percent' in description
> 
> -- A.

Thanks, I've already set the approved flag on the review. If the sponsoring process will require more than 1 month let me know when everything is ready as I will need to refresh the flag.

Comment 58 Fedora Admin user for bugzilla script actions 2024-06-19 07:19:27 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/supernovas

Comment 59 Fedora Update System 2024-06-19 07:52:14 UTC
FEDORA-2024-62fc9e2e9b (supernovas-1.0.1-3.fc41) has been submitted as an update to Fedora 41.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-62fc9e2e9b

Comment 60 Fedora Update System 2024-06-19 07:56:25 UTC
FEDORA-2024-62fc9e2e9b (supernovas-1.0.1-3.fc41) has been pushed to the Fedora 41 stable repository.
If problem still persists, please make note of it in this bug report.


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