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

build against system double-conversion #375

Closed
pgajdos opened this issue Mar 10, 2020 · 6 comments · Fixed by #508
Closed

build against system double-conversion #375

pgajdos opened this issue Mar 10, 2020 · 6 comments · Fixed by #508

Comments

@pgajdos
Copy link

pgajdos commented Mar 10, 2020

Hi,

could you please consider to add a possibility to build against system double-conversion instead of the bundled one? For example this:
https://build.opensuse.org/package/view_file/home:pgajdos:python/python-ujson/python-ujson-system-double-conversion.patch?expand=1
is working for me, but not sure about general solution.

@pgajdos pgajdos changed the title build against double-conversion build against system double-conversion Mar 10, 2020
@musicinmybrain
Copy link
Contributor

That link is now 404, but in Fedora, removing the deps directory and applying this patch is working for me:

diff -Naur ujson-5.1.0-original/setup.py ujson-5.1.0/setup.py
--- ujson-5.1.0-original/setup.py	2021-12-20 08:07:30.000000000 -0500
+++ ujson-5.1.0/setup.py	2022-02-14 11:49:58.791418901 -0500
@@ -18,9 +18,9 @@
         "./lib/ultrajsonenc.c",
         "./lib/ultrajsondec.c",
     ],
-    include_dirs=["./python", "./lib", "./deps/double-conversion/double-conversion"],
+    include_dirs=["./python", "./lib", "/usr/include/double-conversion"],
     extra_compile_args=["-D_GNU_SOURCE"],
-    extra_link_args=["-lstdc++", "-lm"] + strip_flags,
+    extra_link_args=["-lstdc++", "-lm", '-ldouble-conversion'] + strip_flags,
 )
 
 with open("python/version_template.h") as f:

If the maintainers like #507, perhaps a similar approach using environment variables could be applied here.

@bwoodsend
Copy link
Collaborator

I'd be fine with this as long as you're well aware that double-conversion is not a PEP599 whitelisted package meaning that it'll never be allowed on PyPI and you'nn need be very wary w.r.t ABI compatibility if you ever intend to compile on one machine then install that binary on another machine.

@hugovk hugovk assigned hugovk and unassigned hugovk Feb 15, 2022
@hugovk
Copy link
Member

hugovk commented Feb 15, 2022

Alternatively/also, if could be worth bumping the version used here.

Version 2.0.1 or so was added 5 years ago in eb7d894 and hasn't been updated since except for #488.

3.2.0 is newest at https://github.com/google/double-conversion

Fedora has 3.1.5: https://fedora.pkgs.org/35/fedora-x86_64/double-conversion-devel-3.1.5-5.fc35.x86_64.rpm.html

@musicinmybrain
Copy link
Contributor

I'd be fine with this as long as you're well aware that double-conversion is not a PEP599 whitelisted package meaning that it'll never be allowed on PyPI and you'nn need be very wary w.r.t ABI compatibility if you ever intend to compile on one machine then install that binary on another machine.

I think this makes sense. This request is mostly targeted at Linux distribution packaging, where bundling dependencies is generally avoided and there is generally a good (if not always flawless) process for protecting users from ABI compatibility issues in distribution packages.

@musicinmybrain
Copy link
Contributor

Alternatively/also, if could be worth bumping the version used here.

I have prepared a PR to update python-ujson in Fedora to 5.1.0, and I can confirm that all the tests still pass when using the system double-conversion, version 3.1.5.

@bwoodsend
Copy link
Collaborator

Ah you should be fine for Linux distribution packaging. Sure, PR away.

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

Successfully merging a pull request may close this issue.

4 participants