Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1299139 - Review Request: astrometry - Tools from Astrometry.net
Summary: Review Request: astrometry - Tools from Astrometry.net
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW Astronomy-SIG
TreeView+ depends on / blocked
 
Reported: 2016-01-16 13:59 UTC by Mattia Verga
Modified: 2017-07-13 00:40 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-09-25 14:11:01 UTC


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Bugzilla 1470436 None None None Never

Internal Links: 1470436

Description Mattia Verga 2016-01-16 13:59:01 UTC
Spec URL: http://www.coolbits.it/fedora/astrometry.spec
SRPM URL: http://www.coolbits.it/fedora/astrometry-0.65-1.fc23.src.rpm
Description: The astrometry engine will take any image and return the astrometry
world coordinate system (WCS), a standards-based description of the
transformation between image coordinates and sky coordinates.
Fedora Account System Username: mattia

Comment 1 Upstream Release Monitoring 2016-01-16 14:07:28 UTC
lupinix's scratch build of astrometry-0.65-1.fc23.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12574737

Comment 2 Christian Dersch 2016-01-16 14:08:46 UTC
Doesn't build @rawhide :( http://koji.fedoraproject.org/koji/taskinfo?taskID=12574742

Comment 3 Christian Dersch 2016-01-16 14:13:34 UTC
Btw: Are you sure license is BSD with advertising, not normal 3 clause BSD?

Comment 4 Upstream Release Monitoring 2016-01-16 14:22:32 UTC
mattia's scratch build of astrometry-0.65-1.fc23.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12574968

Comment 5 Mattia Verga 2016-01-16 14:37:53 UTC
License text shipped with sources is like this:
https://fedoraproject.org/wiki/Licensing:BSD?rd=Licensing/BSD#BSDwithAdvertising

See LICENSE file in sources main directory.

For the build failure in rawhide: I will check, most probably I forgot to add some buildrequires... locally on a F23 build machine it builds fine.

Comment 6 Mattia Verga 2016-01-16 14:43:08 UTC
...forgot what I said, I messed up with licenses. It's BSD NO advertising, you're right.

Comment 7 Mattia Verga 2016-01-16 16:49:57 UTC
I confirm that building on F23 works, Rawhide fails... reported upstream https://groups.google.com/forum/#!topic/astrometry/aDCjhfMYhpE

Comment 8 Mattia Verga 2016-01-16 17:48:26 UTC
Spec URL: http://www.coolbits.it/fedora/astrometry.spec
SRPM URL: http://www.coolbits.it/fedora/astrometry-0.66-1.fc23.src.rpm

* Sat Jan 16 2016 Mattia Verga <mattia.verga@tiscali.it> - 0.66-1
- Correct license to BSD
- Upgrade to 0.66 fix build on Rawhide

Comment 9 Upstream Release Monitoring 2016-01-16 18:05:17 UTC
mattia's scratch build of astrometry-0.66-1.fc23.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12578072

Comment 10 Mattia Verga 2016-01-17 08:23:08 UTC
Spec URL: http://www.coolbits.it/fedora/astrometry.spec
SRPM URL: http://www.coolbits.it/fedora/astrometry-0.66-2.fc23.src.rpm

* Sun Jan 17 2016 Mattia Verga <mattia.verga@tiscali.it> - 0.66-2
- -devel and -static are not noarch

Comment 11 Upstream Release Monitoring 2016-01-17 08:40:31 UTC
mattia's scratch build of astrometry-0.66-2.fc23.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12584799

Comment 12 Mattia Verga 2016-01-19 13:10:59 UTC
Spec URL: http://www.coolbits.it/fedora/astrometry.spec
SRPM URL: http://www.coolbits.it/fedora/astrometry-0.66-3.fc23.src.rpm

%changelog
* Sun Jan 17 2016 Mattia Verga <mattia.verga@tiscali.it> - 0.66-3
- Add some index files for basic functionality
- Config file should not be overwritten
- Remove static libraries from package

Comment 13 Mattia Verga 2016-02-09 08:35:14 UTC
Spec URL: http://www.coolbits.it/fedora/astrometry.spec
SRPM URL: http://www.coolbits.it/fedora/astrometry-0.66-4.fc23.src.rpm

%changelog
* Sun Jan 24 2016 Mattia Verga <mattia.verga@tiscali.it> - 0.66-4
- Specify NetPBM paths
- Add wcslib BR
- Correct license to add GPLv2+ for index files

Comment 14 Mattia Verga 2016-03-06 11:06:53 UTC
Spec URL: http://www.coolbits.it/fedora/astrometry.spec
SRPM URL: http://www.coolbits.it/fedora/astrometry-0.67-1.fc23.src.rpm

%changelog
* Sun Mar 03 2016 Mattia Verga <mattia.verga@tiscali.it> - 0.67-1
- Update to 0.67
- Use pkgconfig for BR

Comment 15 Upstream Release Monitoring 2016-03-06 11:11:26 UTC
mattia's scratch build of astrometry-0.67-1.fc23.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=13249652

Comment 16 Zbigniew Jędrzejewski-Szmek 2016-03-07 02:42:22 UTC
The Summary doesn't match the description. Just say what the tools do in the Summary.

In the comment about sources, provide explicit instructions how to download the sources, e.g. the arguments to the script if any any.

Would it work with python3? Python3 is preferred. If the python bits are a module to be used externally, you should provide both python2 and python3 versions if both are supported. Also, you should use the %{python_provide} macro [https://fedoraproject.org/wiki/Packaging:Python#The_.25python_provide_macro].

Comment 17 Björn 'besser82' Esser 2016-03-12 09:04:02 UTC
Another thing to mention:  The mandatory %__global_ldflags are *NOT* properly applied during the build…

Since the package uses SWIG to generate the wrappers, you can easily provide a Python2 AND Python3 version, by passing the correct python-libs to link against.

Comment 18 Mattia Verga 2016-03-16 08:09:11 UTC
Thanks for the tips.
I've asked upstream and found that python code is usable externally. I thought it wasn't and that's why I didn't packaged a separate python package.
However, astronomy isn't compatible with python3.

I need some more help about that (python guidelines are not clear to me): I think I should create a subpackage named "astronomy-python2" with a "Provides: python2-astronomy" line, right? The %python_provide macro usage explanation is not clear to me...

Björn, about the %__global_ldflags: aren't they supposed to be applied with the ARCH_FLAGS="%{optflags}" passed to make?

Comment 19 Zbigniew Jędrzejewski-Szmek 2016-03-16 12:04:27 UTC
(In reply to Mattia Verga from comment #18)
> I need some more help about that (python guidelines are not clear to me): I
> think I should create a subpackage named "astronomy-python2" with a
> "Provides: python2-astronomy" line, right?
The guidelines are not very clear. But what you describes sounds like the current standard practice.

> Björn, about the %__global_ldflags: aren't they supposed to be applied with
> the ARCH_FLAGS="%{optflags}" passed to make?
The only thing that counts is whether the expected ldflags are passed on to gcc during build. If the logs indicate that this isn't the case, you must find a different way to pass them.

Comment 20 Mattia Verga 2016-03-17 17:22:43 UTC
Spec URL: http://www.coolbits.it/fedora/astrometry.spec
SRPM URL: http://www.coolbits.it/fedora/astrometry-0.67-3.fc23.src.rpm

%changelog
* Thu Mar 17 2016 Mattia Verga <mattia.verga@tiscali.it> - 0.67-3
- Apply mandatory ld flags

* Wed Mar 16 2016 Mattia Verga <mattia.verga@tiscali.it> - 0.67-2
- Add python2 subpackage

Comment 21 Zbigniew Jędrzejewski-Szmek 2016-03-17 18:26:03 UTC
Build fails in mock for me. Log: https://in.waw.pl/~zbyszek/fedora/astrometry-build-log

Comment 22 Mattia Verga 2016-03-18 06:51:41 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #21)
> Build fails in mock for me. Log:
> https://in.waw.pl/~zbyszek/fedora/astrometry-build-log

Strange, I've succesfully built it in Copr:
https://copr-be.cloud.fedoraproject.org/results/mattia/Astronomy/fedora-rawhide-x86_64/00168982-astrometry/build.log.gz

Seems that ARCH_FLAGS are not the same. In Copr:
make SYSTEM_GSL=yes -j2 'ARCH_FLAGS=-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic'

In your mock:
make SYSTEM_GSL=yes -j12 'ARCH_FLAGS=-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables'

Comment 23 Mattia Verga 2016-03-18 06:53:56 UTC
...sorry, I saw later that your mock was i686.
So the only change is 'make -j2' vs 'make -j12'

Comment 24 Zbigniew Jędrzejewski-Szmek 2016-03-18 12:21:47 UTC
I also made a scratch build in koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=13377711. It failed on amd64 arch.

Comment 25 Zbigniew Jędrzejewski-Szmek 2016-03-18 12:27:18 UTC
-Werror strikes again:
Testing swap qsort_r...
((gcc -Werror -o os-features-test-swap-qsort \
     -g -Wall -ffinite-math-only -fno-signaling-nans -pthread -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -O3 -fomit-frame-pointer -DNDEBUG -fpic -Winline -I../include -I../include/astrometry -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -DAN_GIT_REVISION='"0.67"' -DAN_GIT_DATE='"Mon_Jan_25_11:20:00_2016_-0500"' -DAN_GIT_URL='"https://github.com/dstndstn/astrometry.net"' -I../util -I/usr/include/cairo -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/freetype2 -I/usr/include/libdrm -I/usr/include/libpng16  -I/usr/include/libpng16     -DWCSLIB_EXISTS      -I../include -I../include/astrometry  -I/usr/include/wcslib    -I../include -I../include/astrometry  -I/usr/include/wcslib -I. -DTEST_SWAP_QSORT_R os-features-test.c -Wl,-z,relro -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -Wl,-z,relro -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -g -Wall -ffinite-math-only -fno-signaling-nans -pthread -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -O3 -fomit-frame-pointer -DNDEBUG -fpic -Winline >> os-features.log && \
   ./os-features-test-swap-qsort >> os-features.log && \
   echo "#define NEED_SWAP_QSORT_R 0") \
|| echo "#define NEED_SWAP_QSORT_R 1") >> ../include/astrometry/os-features-config.h.tmp
os-features-test.c: In function 'main':
os-features-test.c:68:36: error: passing argument 4 of 'qsort_r' from incompatible pointer type [-Werror=incompatible-pointer-types]
     qsort_r(array, N, sizeof(int), &mythunk, sortfunc);
                                    ^
In file included from os-features-test.c:5:0:
/usr/include/stdlib.h:767:13: note: expected '__compar_d_fn_t {aka int (*)(const void *, const void *, void *)}' but argument is of type 'int *'
 extern void qsort_r (void *__base, size_t __nmemb, size_t __size,
             ^~~~~~~
cc1: all warnings being treated as errors



There's also a bunch of interesting warnings:
kdtree_internal.c: In function 'kdtree_build_2_fff':
kdtree_internal.c:2247:2: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
  kd->lr[0] = N - 1;
  ^~
kdtree_internal.c:2243:5: note: ...this 'if' clause, but it is not
     if (options & KD_BUILD_LINEAR_LR)
     ^~

Might be worth looking into.

Comment 26 Mattia Verga 2016-03-18 17:36:31 UTC
The problem is that F25 seems to have set parallel building to 16 concurrent threads (make -j16) while F24 had 4 (make -j4) and this doesn't like to astrometry package.
In fact, in F24 astrometry builds fine:
http://koji.fedoraproject.org/koji/taskinfo?taskID=13383841

By forcing the -j4 flag it also builds in rawhide:
http://koji.fedoraproject.org/koji/taskinfo?taskID=13384253

Now, since the Guidelines say "Whenever possible, invocations of make should be done as make %{?_smp_mflags}", I think I can use that custom value instead of default macro to overcome the problem, right?

Comment 27 Zbigniew Jędrzejewski-Szmek 2016-03-18 19:21:34 UTC
Yes, you can. There's probably a bug in the build system, but not every bug has to be fixed (especially when it's in the build system, so it doesn't affect users).

Comment 28 Zbigniew Jędrzejewski-Szmek 2016-03-18 20:06:10 UTC
Oh, one more note:
> F25 seems to have set parallel building to 16 concurrent threads 

This depends on the builder, not on the version. It's possible that rawhide get's bigger builders instead of slower smaller builders. There was some talk of new arm64 machines, I wonder if those are those.

Comment 29 Mattia Verga 2016-03-19 11:05:40 UTC
Spec URL: http://www.coolbits.it/fedora/astrometry.spec
SRPM URL: http://www.coolbits.it/fedora/astrometry-0.67-4.fc23.src.rpm

%changelog
* Sat Mar 19 2016 Mattia Verga <mattia.verga@tiscali.it> - 0.67-4
- Limit parallel make at 4 processes to fix build on F25
- Remove odd symlink in sources
- Fix wrong FSF address

I removed an odd link in sources main directory because it breaks the license check part of fedora-review.
About licenses (and the wrong FSF address fix) I've asked upstream to fix source headers, because the project was relicensed under BSD, but licenses in source headers are a mess:
https://groups.google.com/forum/#!topic/astrometry/mCuyze3TOeM

Comment 30 Mattia Verga 2016-03-20 08:40:36 UTC
Spec URL: http://www.coolbits.it/fedora/astrometry.spec
SRPM URL: http://www.coolbits.it/fedora/astrometry-0.67-5.fc23.src.rpm

* Sun Mar 20 2016 Mattia Verga <mattia.verga@tiscali.it> - 0.67-5
- Remove patented catalog and pictures from sources

Ole Streicher <olebole@debian.org> from Debian pointed out that a catalog in sources is not free (free only for personal use), so I removed that.
I also removed demo files because their licenses are unclear.

Comment 31 Mattia Verga 2016-03-20 08:42:13 UTC
Spec URL: http://www.coolbits.it/fedora/astrometry.spec
SRPM URL: http://www.coolbits.it/fedora/astrometry-0.67-5.fc24.src.rpm

Sorry, bad link.

Comment 32 Zbigniew Jędrzejewski-Szmek 2016-03-20 14:55:27 UTC
libwcs has Summary "An implementation of the FITS World Coordinate System standard". I think astrometry should use the similar spelling for consistency (and abbrevs are best avoided):
Summary: Get World Coordinate System coordinates from an image

astrometry.x86_64: W: spelling-error %description -l en_US convertion -> conversion

The main package requires the python subpackage, and the python subpackage requires the main package. This defeats the splitting out of the python subpackage a bit, but it's OK (for example if python3 support is added in the future it will be useful). So nothing to change here, just taking note.

OTOH, the data package is not required by any of the other subpackages. Do other subpackages work without the -data subpackge? I'd expect the main subpackage to Require -data.

Calls to ldconfig are missing [https://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries].

File /usr/bin/tabmerge from install of astrometry-0.67-5.fc23.x86_64 conflicts with file from package perl-Text-RecordParser-1.6.5-3.fc23.noarch.
That's a sign of a bigger problem. Many binaries in astrometry have very generic names: tablist, tabmerge, tabsort, plotxy, imstat, imcopy, listhead, liststruc, modhead, pad-file, subtable. tabmerge has to be renamed, but I think that the other binaries with "generic" names should be renamed too. Would an "astrometry-" or "wcs-" prefix work?

For the user it usually does not matter that executables are written in Python. Maybe you should drop the ".py" suffixes on executables in astrometry-python2?

Comment 33 Christian Dersch 2016-09-25 12:17:26 UTC
Is this one still alive? Links for SRPM and spec are no longer working.

Comment 34 Mattia Verga 2016-09-25 14:11:01 UTC
Sorry, I'm not planning to continue with this one. If someone wants to take it over, the SRPM and spec are available on copr under Mattia/Astronomy (the domain coolbits.it is dead).

I will go ahead and follow the policy for a stalled package review by closing this bug.

Comment 35 Christian Dersch 2017-07-13 00:40:45 UTC
Opened a new review based on your packaging effort, fixing some things like conflicting names and unversioned soname.


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