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

Run manylinux tests using current python version #199

Merged
merged 1 commit into from
Oct 9, 2019

Conversation

mayeut
Copy link
Member

@mayeut mayeut commented Oct 3, 2019

Run integration tests on manylinux using the python version used to test auditwheel.
Since travis-ci is running 3.5, 3.6 & 3.7, we're better off testing those versions inside manylinux containers rather than testing 3 times the exact same thing.

@codecov
Copy link

codecov bot commented Oct 3, 2019

Codecov Report

Merging #199 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #199   +/-   ##
=======================================
  Coverage   87.43%   87.43%           
=======================================
  Files          19       19           
  Lines         963      963           
  Branches      210      210           
=======================================
  Hits          842      842           
  Misses         84       84           
  Partials       37       37

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 ba71e6f...d1bdb72. Read the comment docs.

Copy link
Contributor

@lkollar lkollar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few suggestions.

DEVTOOLSET = {
'manylinux1': 'devtoolset-2',
'manylinux2010': 'devtoolset-8',
}
PATH_DIRS = [
'/opt/python/cp35-cp35m/bin',
'/opt/python/{}/bin'.format(PYTHON_ABI),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to make this dynamic, you could just do os.path.dirname(sys.executable).

Copy link
Member Author

@mayeut mayeut Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't if I'm not mistaken. sys.executable relates to the python interpreter running the tests while /opt/python/{}/bin'.format(PYTHON_ABI) relates to the python interpreter running in the manylinux container.

@@ -302,7 +305,7 @@ def test_build_wheel_with_image_dependencies(with_dependency, any_manylinux_cont
assert 'manylinux' not in orig_wheel

repair_command = \
'LD_LIBRARY_PATH=/auditwheel_src/tests/integration/testdependencies '\
'LD_LIBRARY_PATH=/auditwheel_src/tests/integration/testdependencies:$LD_LIBRARY_PATH '\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems unrelated. It's probably harmless, but I wonder why you are changing it.

Copy link
Member Author

@mayeut mayeut Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When running python 3.7 tests, libcrypto.so.2 needs to be found. Overwriting LD_LIBRARY_PATH not preserving the existing one prevents libcrypto.so.2 to be found.

@@ -436,21 +439,21 @@ def test_build_wheel_depending_on_library_with_rpath(any_manylinux_container, do
).format(policy=policy, orig_wheel=orig_wheel)
docker_exec(
manylinux_ctr,
['bash', '-c', 'LD_LIBRARY_PATH=/auditwheel_src/tests/integration/testrpath/a ' + repair_command],
['bash', '-c', 'LD_LIBRARY_PATH=/auditwheel_src/tests/integration/testrpath/a:$LD_LIBRARY_PATH ' + repair_command],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem

@@ -23,13 +24,15 @@
'manylinux2010': MANYLINUX2010_IMAGE_ID,
}
DOCKER_CONTAINER_NAME = 'auditwheel-test-manylinux'
PYTHON_IMAGE_ID = 'python:3.5'
PYTHON_MAJ_MIN = [str(i) for i in sys.version_info[:2]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another approach to this would be to use the wheel package which we already depend on. That should provide a better guess at the ABI tag as it handles things like figuring out the allocator, etc.

from wheel.pep425tags import get_abbr_impl, get_impl_ver, get_abi_tag
PYTHON_ABI = "{0}{1}-{2}".format(get_abbr_impl(), get_impl_ver(), get_abi_tag()))

Copy link
Member Author

@mayeut mayeut Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't if I'm not mistaken. sys.executable relates to the python interpreter running the tests while /opt/python/{}/bin'.format(PYTHON_ABI) relates to the python interpreter running in the manylinux container.

This is also valid here I think but if there's something better, please correct me.

@auvipy auvipy merged commit 3dd26fe into pypa:master Oct 9, 2019
@mayeut mayeut deleted the manylinux-python-version branch October 9, 2019 20:19
@di di mentioned this pull request Nov 5, 2019
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 this pull request may close these issues.

3 participants