Bug 2283055
Description
Attila Kovacs
2024-05-23 19:38:24 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. *** Bug 2282872 has been marked as a duplicate of this bug. *** *** Bug 2282877 has been marked as a duplicate of this bug. *** *** Bug 2282874 has been marked as a duplicate of this bug. *** 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 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. Spec URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-aarch64/07485461-supernovas/supernovas.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-aarch64/07485461-supernovas/supernovas-1.0.1-1.fc41.src.rpm - Fixes for the issues noted in Comment 5. -- Attila Created attachment 2034987 [details]
The .spec file difference from Copr build 7482753 to 7485576
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. 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. 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. 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 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. 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. Created attachment 2035167 [details]
The .spec file difference from Copr build 7492432 to 7492436
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. 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. Created attachment 2035277 [details]
The .spec file difference from Copr build 7492436 to 7494765
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. 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 Created attachment 2035355 [details]
The .spec file difference from Copr build 7494765 to 7496349
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. 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 Created attachment 2035372 [details]
The .spec file difference from Copr build 7496349 to 7496545
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. 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). 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. 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 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. Spec URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07512039-supernovas/supernovas.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/attipaci/supernovas/fedora-rawhide-x86_64/07512039-supernovas/supernovas-1.0.1-2.fc41.src.rpm Created attachment 2036275 [details]
The .spec file difference from Copr build 7496545 to 7528738
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. 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}? 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. (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. (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/ (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. > > > - 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. > > 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. > %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. > 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
(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 (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. 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. Created attachment 2036924 [details]
The .spec file difference from Copr build 7528738 to 7585895
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. 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.
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. > 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.
Yes, that's better practice, I agree :-). 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 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 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. 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. (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 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. (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. The Pagure repository was created at https://src.fedoraproject.org/rpms/supernovas 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 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. |