ionelmc / tox-wheel

A Tox plugin that builds and installs wheels instead of sdist. Note that this plugin is obsolte as tox 4.0 already has wheel support.
BSD 2-Clause "Simplified" License
23 stars 10 forks source link

Fix skip_missing_interpreters issue wheel+sdist building #14

Closed teruokun closed 2 years ago

teruokun commented 2 years ago

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
codecov[bot] commented 2 years ago

Codecov Report

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

:exclamation: 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%) :arrow_up:
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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

teruokun commented 2 years ago

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

teruokun commented 2 years ago

Awesome!

On Tue, Mar 1, 2022 at 3:46 AM Ionel Cristian Mărieș < @.***> wrote:

Merged #14 https://github.com/ionelmc/tox-wheel/pull/14 into master.

— Reply to this email directly, view it on GitHub https://github.com/ionelmc/tox-wheel/pull/14#event-6161467914, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYCRPM3BFBYZOKBFCCAATTU5X7SDANCNFSM5E3RNXBA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

ionelmc commented 2 years ago

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 commented 2 years ago

Oh sorry! I’m a little new to getting GitHub/git to give me the right result here. What can I do from my side?

On Tue, Mar 1, 2022 at 3:51 AM Ionel Cristian Mărieș < @.***> wrote:

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 :-)

— Reply to this email directly, view it on GitHub https://github.com/ionelmc/tox-wheel/pull/14#issuecomment-1055346912, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYCRPNDDDYQOISPJN3WKFTU5YAF5ANCNFSM5E3RNXBA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

ionelmc commented 2 years ago

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

ionelmc commented 2 years ago

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

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