Skip to content

add udunits xml to wheel #191

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.

Sign up for GitHub

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

Jump to bottom
Merged
ocefpaf merged 3 commits into from
Jul 28, 2021
Merged

add udunits xml to wheel #191

ocefpaf merged 3 commits into from
Jul 28, 2021

Conversation

@bjlittle
Copy link
Member

@bjlittle bjlittle commented Jul 24, 2021 •
edited
Loading

🚀 Pull Request

Description

This PR supports #186, by injecting the associated UDUNITS2 xml files into a wheel build of cf-units.

@ocefpaf To build in a quay.io/pypa/manylinux2014_x86_64 docker container, I had to do the following:

yum install -y udunits2-devel

export UDUNITS2_INCDIR="/usr/include/udunits2"
export UDUNITS2_LIBDIR="/usr/lib64"
export UDUNITS2_XML_PATH="/usr/share/udunits/udunits2.xml"

export PYBIN="/opt/python/cp38-cp38/bin/python"

$PYBIN -m pip install --upgrade pip wheel setuptools setuptools_scm build twine
$PYBIN -m build --sdist --wheel . --outdir /io/wheelhouse/

auditwheel repair /io/wheelhouse/*.whl

Note that, the UDUNITS2 XML files populate the etc/share directory only for a bdist_wheel and not a sdist.

When a wheel installed cf-units distro executes it auto-generates an ephemeral site.cfg pointing to the bundled XML files, and the rest of the machinery works as normal. The wheel should be consistent with regards to udunits2, as the wheel bundles the udunits2 dynamic library associated with the etc/share XML files

@codecov
Copy link

codecov bot commented Jul 24, 2021 •
edited
Loading

Codecov Report

Merging #191 (4abe85a) into main (2caad34) will increase coverage by 0.32%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #191      +/-   ##
==========================================
+ Coverage   90.63%   90.96%   +0.32%     
==========================================
  Files           7        6       -1     
  Lines         833      819      -14     
  Branches      105      103       -2     
==========================================
- Hits          755      745      -10     
+ Misses         64       62       -2     
+ Partials       14       12       -2

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 2caad34...4abe85a. Read the comment docs.

bjlittle
bjlittle commented Jul 24, 2021
View reviewed changes
bjlittle
bjlittle commented Jul 24, 2021
View reviewed changes
@bjlittle bjlittle requested a review from ocefpaf July 24, 2021 01:11
@bjlittle bjlittle added New: Pull Request Highlight a new community raised pull-request Type: Infrastructure labels Jul 24, 2021
bjlittle
bjlittle commented Jul 24, 2021
View reviewed changes
@bjlittle bjlittle mentioned this pull request Jul 24, 2021
Closed
@bjlittle bjlittle marked this pull request as draft July 24, 2021 12:30
@bjlittle
Copy link
Member Author

bjlittle commented Jul 24, 2021 •
edited
Loading

@ocefpaf Okay, so I've put this in draft for the moment, as I've discovered a way to avoid the use of the CFUNITS_WHEEL environment variable, which was a bit unpleasant to be honest, and I want entirely happy using that kind of approach.

I'll add my changes and test before taking this out of draft again 👍

@bjlittle bjlittle marked this pull request as ready for review July 25, 2021 00:41
@bjlittle
Copy link
Member Author

bjlittle commented Jul 25, 2021

@ocefpaf Okay, this is good to review now... fancy taking a peek?

@bjlittle bjlittle mentioned this pull request Jul 26, 2021
Merged
@ocefpaf
Copy link
Member

ocefpaf commented Jul 26, 2021

@ocefpaf Okay, this is good to review now... fancy taking a peek?

LGTM!

@bjlittle
Copy link
Member Author

bjlittle commented Jul 27, 2021

You've got merge rights @ocefpaf, right?

@ocefpaf
Copy link
Member

ocefpaf commented Jul 28, 2021

You've got merge rights @ocefpaf, right?

I believe I do but merging is blocked. I guess we need 1 approving review.

@ocefpaf ocefpaf merged commit 5540e08 into SciTools:main Jul 28, 2021
@bjlittle
Copy link
Member Author

bjlittle commented Jul 28, 2021

@ocefpaf Awesome, nice one 🥇

@bjlittle bjlittle deleted the add-xml-for-wheels branch August 11, 2021 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ocefpaf ocefpaf ocefpaf approved these changes

Assignees

@ocefpaf ocefpaf

Labels

New: Pull Request Highlight a new community raised pull-request Type: Infrastructure

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bjlittle @ocefpaf