spherical_geometry icon indicating copy to clipboard operation
spherical_geometry copied to clipboard

TST: Add a job to use system lib qd

Open pllim opened this issue 2 years ago • 14 comments

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:

pllim avatar Oct 31 '23 01:10 pllim

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.

mcara avatar Oct 31 '23 02:10 mcara

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

Universebenzene avatar Oct 31 '23 03:10 Universebenzene

@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

pllim avatar Oct 31 '23 17:10 pllim

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

mcara avatar Oct 31 '23 18:10 mcara

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?

pllim avatar Oct 31 '23 18:10 pllim

@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.

Universebenzene avatar Nov 01 '23 04:11 Universebenzene

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.

pllim avatar Nov 01 '23 16:11 pllim

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 avatar Nov 01 '23 22:11 Hellseher

Here is a quick response from the maintainer of the project David Bailey [email protected]

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

Hellseher avatar Nov 01 '23 23:11 Hellseher

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 .

pllim avatar Nov 02 '23 04:11 pllim

qd-2.3.24 should solve all the problem.

Universebenzene avatar Nov 03 '23 16:11 Universebenzene

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

pllim avatar Nov 03 '23 19:11 pllim

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 [email protected]

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

DHB

<qd-fix-accuracy-in-angle-computation.patch>

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

Hellseher avatar Nov 03 '23 22:11 Hellseher

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

pllim avatar Nov 07 '23 16:11 pllim