Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
pllim opened this issue Oct 31, 2023 · 14 comments
Open

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

pllim opened this issue Oct 31, 2023 · 14 comments

Comments

@pllim
Copy link
Contributor

pllim commented Oct 31, 2023

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 pllim changed the title TST: Add a job to use system libqd TST: Add a job to use system lib qd Oct 31, 2023
@mcara
Copy link
Member

mcara commented Oct 31, 2023

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
Copy link

Universebenzene commented Oct 31, 2023

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

@pllim
Copy link
Contributor Author

pllim commented Oct 31, 2023

@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
Copy link
Member

mcara commented Oct 31, 2023

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

else:
if have_windows:
qdpath = os.environ.get('QD_PATH', '')
if not qdpath:
print('WINDOWS USERS:\n\n'
'Define QD_PATH to the prefix where "qd" '
'is installed:\n\n'
'set QD_PATH="x:\\prefix\\of\\qd"\n\n'
'Expected directory structure of QD_PATH:\n'
' QD_PATH\\lib\n'
' QD_PATH\\include\n\n', file=sys.stderr)
exit(1)
qdpath = os.path.abspath(qdpath)
if arg == 'libs':
result = '/LIBPATH:' + os.path.join(qdpath, 'lib')
result += ' qd.lib'
elif arg == 'cflags':
result = '-I' + os.path.join(qdpath, 'include')
else:
print('Unsupported option: {}'.format(arg), file=sys.stderr)
exit(1)

@pllim
Copy link
Contributor Author

pllim commented Oct 31, 2023

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
Copy link

@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
Copy link
Contributor Author

pllim commented Nov 1, 2023

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
Copy link

Hellseher commented Nov 1, 2023

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 astropy/astropy#14311

@Hellseher
Copy link

Hellseher commented Nov 1, 2023

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

@pllim
Copy link
Contributor Author

pllim commented Nov 2, 2023

According to #227 (comment) , it was a patch from @mcara in #224 .

@Universebenzene
Copy link

qd-2.3.24 should solve all the problem.

@pllim
Copy link
Contributor Author

pllim commented Nov 3, 2023

I hope so. Please see #258 . Mihai is a good man.

@Hellseher
Copy link

Hellseher commented Nov 3, 2023

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

@pllim
Copy link
Contributor Author

pllim commented Nov 7, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants