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

Fix skip_missing_interpreters issue wheel+sdist building #14

Merged
merged 31 commits into from Mar 1, 2022

Conversation

teruokun
Copy link

When building both wheels and sdists, the preference to a built sdist if it already exists causes issues when attempting to test the coherency of both wheel and sdist artifacts on multiple interpreters.

This patch handles 2 issues associated with wheel+sdist builds:

  1. Take into account the wheel configuration preferentially so they are treated and tested independently
  2. Handle skip_missing_interpreters properly when an interpreter is missing as this is run during the packaging phase and so can be passed environments with untested interpreters, causing a failed build even if skip_missing_interpreters is set

Testing

Tested in an environment with the following tox.ini both with and without the skip_missing_interpreters flag (py33 does not exist in this system, but 37,38 and 39 all do):

[tox]
isolated_build = true
envlist = py{37,38,39,33}-{whl,sdist}
skip_missing_interpreters = true

[testenv]
commands = pytest \
    --cov "{envsitepackagesdir}/sample_broadly_consumed_library" \
    --cov-config "{toxinidir}/pyproject.toml" \
    {posargs:tests/}
wheel =
    whl: true
    sdist: false
extras = testing

@@ -61,15 +62,21 @@ def patch(obj, attr, value):

@hookimpl
def tox_package(session, venv):
if hasattr(session, "package"):
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this part still be here?

Copy link
Author

Choose a reason for hiding this comment

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

It is as it's moved to be an elif below the wheel consideration as it looked like session.package is set if it ever builds the sdist. The key is to imagine a binary package that is generating binary wheels for some interpreters, but otherwise wants an sdist available for platforms they may not have support to build wheels for themselves (i.e. not being able to build for windows or OSX for example). If you get the order wrong in any way where any sdist is built before any of the wheels, then the remaining wheel builds/tests will no longer considering building a wheel even if it explicitly says in the envconfig that it should be building and testing a wheel

Copy link
Author

Choose a reason for hiding this comment

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

Any additional comments or concerns?

@codecov
Copy link

codecov bot commented Oct 4, 2021

Codecov Report

Merging #14 (033ed86) into master (7e7f451) will increase coverage by 2.84%.
The diff coverage is 100.00%.

❗ Current head 033ed86 differs from pull request most recent head f9e4f61. Consider uploading reports for the commit f9e4f61 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #14      +/-   ##
==========================================
+ Coverage   81.92%   84.76%   +2.84%     
==========================================
  Files           3        3              
  Lines         177      210      +33     
  Branches       14       15       +1     
==========================================
+ Hits          145      178      +33     
  Misses         28       28              
  Partials        4        4              
Impacted Files Coverage Δ
src/tox_wheel/plugin.py 68.00% <100.00%> (+2.04%) ⬆️
tests/test_tox_wheel.py 100.00% <100.00%> (ø)

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 7e7f451...f9e4f61. Read the comment docs.

@teruokun
Copy link
Author

teruokun commented Oct 4, 2021

Sorry for the large number of commits to fix thing. The Appveyor configuration looks like it may have been a bit dated (was getting reports of missing easy_install and pip not supporting progress bar. But I included those fixes since I didn't want them to have to be overridden to get this change considered

@teruokun
Copy link
Author

teruokun commented Oct 7, 2021

Any further comments on this pull request? I'd love to see these bug-fixes added in as I'd prefer to use the official tox-wheel module over a self-patched version to get these features (it's simpler from a long-term maintenance standpoint)

@teruokun
Copy link
Author

Could I get some feedback on this please? The issue regarding not properly handling skip_missing_interpreters is currently blocking usage of the public tox-wheel package which I'd much rather do than maintain a fork just for this functionality

@ionelmc ionelmc mentioned this pull request Feb 7, 2022
@ionelmc
Copy link
Owner

ionelmc commented Feb 7, 2022

Ooof, I forgot about this. Well it looks fine, can you rebase it? I switched to different CI (travis is dead unfortunately).

@teruokun
Copy link
Author

teruokun commented Feb 7, 2022

Alright, I think it's updated, though I think the tox actions are wrong because you don't seem to have a docs tox environment declared AFAICT in the tox.init

@ionelmc ionelmc merged commit 97b5df1 into ionelmc:master Mar 1, 2022
@teruokun
Copy link
Author

teruokun commented Mar 1, 2022 via email

@ionelmc
Copy link
Owner

ionelmc commented Mar 1, 2022

oooof... I just noticed that there wasn't any rebase done, just a merge. Now we have two merges I can't trust damn it :-)

@teruokun
Copy link
Author

teruokun commented Mar 1, 2022 via email

@ionelmc
Copy link
Owner

ionelmc commented Mar 1, 2022

Nothing. I need to pay more attention 🤦‍♂️

@ionelmc
Copy link
Owner

ionelmc commented Mar 1, 2022

So I've made this squash right here 487f731 to clean things up.

I hope no one noticed the evil stuff I've done to the master branch :-)

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.

None yet

3 participants