spacetelescope / spherical_geometry

A Python package for handling spherical polygons that represent arbitrary regions of the sky
http://spherical-geometry.readthedocs.io/
61 stars 31 forks source link

TST: Add a job to use system lib qd #255

Open pllim opened 10 months ago

pllim commented 10 months ago

There are a number of bug reports about failure to use this packages with system libqd. We should add a job to test this package using system libqd instead of the bundled version.

Looks like it is available on Ubuntu: https://ubuntu.pkgs.org/20.04/ubuntu-universe-amd64/libqd-dev_2.3.22+dfsg.1-3build1_amd64.deb.html

Also see:

mcara commented 10 months ago

System libqd IS NOT supported (anymore). The code is simply incompatible with the original libqd. The bundled libqd has a different API. It is not going to work.

Universebenzene commented 10 months ago

System version of qd needs to be patched as this, which is simply converted from the changing of this repo.

pllim commented 10 months ago

@mcara , if there is no plan to support system qd, then this doc needs to be updated:

https://spherical-geometry.readthedocs.io/en/latest/spherical_geometry/user.html#requirements

mcara commented 10 months ago

Yes, I am aware of that docstring. I am just waiting to see how to proceed and what would be a preferred way to deal with this. For example, we could also get rid of https://github.com/spacetelescope/spherical_geometry/blob/3ba02d9205e5642f4ca66b431cc280349dc7f248/setup.py#L29-L50

pllim commented 10 months ago

Yeah, I suspect all the code to use system qd can be removed. In their place, there would be an error message saying that system qd request is being ignored and we are using the bundled version regardless.

If you use the bundled qd, and there is a system qd, does the bundled version overwrite the system version? Or they live in their own worlds?

Universebenzene commented 10 months ago

@mcara , if there is no plan to support system qd, then this doc needs to be updated:

https://spherical-geometry.readthedocs.io/en/latest/spherical_geometry/user.html#requirements

I think it should be OK to use patched system qd, so the variable USE_SYSTEM_QD could be remained.

pllim commented 10 months ago

I think it should be OK to use patched system qd

I am not sure if there is a way for this package to know if your system qd is patched or not until it is too late. I guess we could add a giant warning in the doc that no one probably reads.

Hellseher commented 10 months ago

If you use the bundled qd, and there is a system qd, does the bundled version overwrite the system version? Or they live in their own worlds?

In my case, Guix does not mix sources but can cross link them if the source is mentioned as an input to the package. It also helps to reproduce environment bit-to-bit on other machines and secure supply chain, it's one the reason to use tested system packages over vendored, patched or bundled.

This case is similary to recent Astropy dropping support of cfitsio and relay on bundeled one https://github.com/astropy/astropy/pull/14311

Hellseher commented 10 months ago

Here is a quick response from the maintainer of the project David Bailey davidhbailey20@gmail.com

It has been many years since I was involved with this code (and I did not write the C++ version in the beginning), but I am open to changes.

Could you do me a favor and send a list of which specific changes you would like to make?

DHB

@pllim @Universebenzene I'd proposed the mentioned patch. Feel free to rise any issues directly with him :)

Thanks, Oleg

pllim commented 10 months ago

According to https://github.com/spacetelescope/spherical_geometry/issues/227#issuecomment-1783769741 , it was a patch from @mcara in https://github.com/spacetelescope/spherical_geometry/pull/224 .

Universebenzene commented 10 months ago

qd-2.3.24 should solve all the problem.

pllim commented 10 months ago

I hope so. Please see https://github.com/spacetelescope/spherical_geometry/pull/258 . Mihai is a good man.

Hellseher commented 10 months ago

Hi

The proposed patch was accepted by the current maintainer of qd, I've noticed that new version has not configure file updated with to reflect it but it's already published.

I've requested to update the version number in configure file so it may be checked during the linking process.

Responce from David (I'm not sure he uses GitHub)

from: | David Bailey davidhbailey20@gmail.com

I have made this change and placed the revised version on the website. Could you check this? https://www.davidhbailey.com/dhbsoftware/

DHB

I've adjusted patch (see attachment) to make it working (removed all tab->space changes which failed to be applied for some reason)

Thanks, Oleg

pllim commented 10 months ago

To use a runner that Action provides, probably have to wait for something like this to catch up first:

https://ubuntu.pkgs.org/20.04/ubuntu-universe-amd64/libqd-dev_2.3.22+dfsg.1-3build1_amd64.deb.html