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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add udunits xml to wheel #191

Merged
merged 3 commits into from Jul 28, 2021
Merged

Conversation

bjlittle
Copy link
Member

@bjlittle bjlittle commented Jul 24, 2021

馃殌 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

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.

setup.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
@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
setup.py Show resolved Hide resolved
@bjlittle bjlittle marked this pull request as draft July 24, 2021 12:30
@bjlittle
Copy link
Member Author

bjlittle commented Jul 24, 2021

@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

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

@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

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

@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
Labels
New: Pull Request Highlight a new community raised pull-request Type: Infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants