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

Support dynamically linking against system double-conversion library #508

Merged
merged 6 commits into from Feb 17, 2022

Conversation

musicinmybrain
Copy link
Contributor

@musicinmybrain musicinmybrain commented Feb 16, 2022

New environment variables UJSON_BUILD_DC_INCLUDES and UJSON_BUILD_DC_LIBS allow overriding the include path for double-conversion and adding linker flags for an external double-conversion library. They should generally be used together.

This is particularly useful to Linux distribution packagers who would otherwise need a patch to unbundle double-conversion. (It may still be necessary to rm -rf deps/double-conversion; I have not checked, as Fedora Linux would require this anyway.)

Fixes #375

Changes proposed in this pull request:

  • Allow overriding default include path/paths for double-conversion with environment variable UJSON_BUILD_DC_INCLUDES. This allows multiple whitespace-delimited paths but as a consequence doesn’t support paths containing whitespace, which is probably acceptable for the intended uses. For example, Linux distributions building against a system copy of double-conversion will want something like UJSON_BUILD_DC_INCLUDES=/usr/include/double-conversion.
  • Allow adding linker flags for double-conversion with environment variable UJSON_BUILD_DC_LIBS; by default there are still none. For example, Linux distributions building against a system copy of double-conversion will want something like UJSON_BUILD_DC_LIBS=-ldouble-conversion.

This fixes ultrajson#376 and is useful to Linux distribution packagers.

New environment variables UJSON_BUILD_DC_INCLUDES and UJSON_BUILD_DC_LIBS allow overriding the include path for double-conversion and adding linker flags for an external double-conversion library. They should generally be used together.
@bwoodsend
Copy link
Collaborator

Could you add a little Build Options section to the end of the readme to illustrate to others how to link against a system double-conversion?

@musicinmybrain
Copy link
Contributor Author

Yes. I can add that to this PR.

setup.py Outdated Show resolved Hide resolved
musicinmybrain and others added 4 commits February 16, 2022 16:53
Automatically disable building ./deps/double-conversion when UJSON_BUILD_DC_LIBS is set to a non-empty value.
@musicinmybrain
Copy link
Contributor Author

Made some changes based on feedback. Ensured the bundled double-conversion is not built if UJSON_BUILD_DC_LIBS is set to a non-empty value. All further feedback is welcome.

setup.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #508 (4688286) into main (7f269a4) will not change coverage.
The diff coverage is n/a.

❗ Current head 4688286 differs from pull request most recent head 9ee6cbb. Consider uploading reports for the commit 9ee6cbb to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #508   +/-   ##
=======================================
  Coverage   88.88%   88.88%           
=======================================
  Files           6        6           
  Lines        1709     1709           
=======================================
  Hits         1519     1519           
  Misses        190      190           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f269a4...9ee6cbb. Read the comment docs.

@bwoodsend bwoodsend changed the title Add env vars to build with system double-conversion Support dynamically linking against system double-conversion library Feb 17, 2022
@bwoodsend bwoodsend added the changelog: Added For new features label Feb 17, 2022
@bwoodsend bwoodsend merged commit fbae6a3 into ultrajson:main Feb 17, 2022
@bwoodsend
Copy link
Collaborator

Do you need ujson to release this before you can go back to your work on the Fedora RPM?

@musicinmybrain
Copy link
Contributor Author

Do you need ujson to release this before you can go back to your work on the Fedora RPM?

Not really—we can easily carry it as a downstream patch for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: Added For new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build against system double-conversion
3 participants