Make pip wheel cache what it built

[sbidoul]

This PR make pip wheel behave like pip install wrt caching of wheels it built locally.

This particularly helps performance in workflows where pip wheel is used for building before installing as documented here.

Closes #6852

[chrahunt]

Just a few comments. I'm OK with the purpose of the change, especially since the old behavior is possible by adding a flag.

I this this could be cleaner if the logic was extracted into the individual calling locations, but I don't think it's worth making that approach a prereq of this functional change.

[sbidoul]

this could be cleaner if the logic was extracted into the individual calling locations

I wanted to keep the diff small, but yes, I fully agree this build method needs more refactoring love. I'd be happy to do some in a followup PR - although I admit I lack inspiration on how to do it right now.

[sbidoul]

I've no clue as to why these macOS checks fail...

[chrahunt]

_______________ test_force_reinstall_with_same_version_specifier _______________
[gw3] darwin -- Python 2.7.16 /Users/runner/runners/2.159.2/work/1/s/.tox/py/bin/python

script = <tests.lib.PipTestEnvironment object at 0x1095b7390>

    def test_force_reinstall_with_same_version_specifier(script):
        """
        Check --force-reinstall when the version specifier equals the installed
        version and the installed version is not the newest version.
        """
>       check_force_reinstall(script, 'simplewheel==1.0', '1.0')

tests/functional/test_install_force_reinstall.py:46: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

script = <tests.lib.PipTestEnvironment object at 0x1095b7390>
specifier = 'simplewheel==1.0', expected = '1.0'

    def check_force_reinstall(script, specifier, expected):
        """
        Args:
          specifier: the requirement specifier to force-reinstall.
          expected: the expected version after force-reinstalling.
        """
        result = script.pip_install_local('simplewheel==1.0')
        check_installed_version(script, 'simplewheel', '1.0')
    
        result2 = script.pip_install_local('--force-reinstall', specifier)
>       assert result2.files_updated, 'force-reinstall failed'
E       AssertionError: force-reinstall failed
E       assert {}
E        +  where {} = <tests.lib.TestPipResult object at 0x10ad47e50>.files_updated

tests/functional/test_install_force_reinstall.py:26: AssertionError

Also not sure - it doesn't seem like something that would be environment-dependent or related to the current change. The set of files updated in a normal run are venv/lib/pythonX.Y/site-packages, venv/lib/pythonX.Y/site-packages/simplewheel, and venv/lib/pythonX.Y/site-packages/simplewheel/__init__.py.

[sbidoul]

I forced a new check and it's green now. Looks like it was a transient error.

[sbidoul]

Here is a commit with some minor refactoring of WheelBuilder.build. Let me know if you want it in this PR or another one.

[chrahunt]

I would have comments on that (like minimizing the scope of the try: ... except: ...) so in the interest of getting this in probably best to leave it out. :)

[chrahunt]

I think if you rebase on master it should fix the packaging issues - Azure Pipelines is using the pre-merge pipeline but the post-merge code which has updates from #7286 which removes generate_authors.

[sbidoul]

Rebased, squashed and green.

[sbidoul]

Many thanks!

[sbidoul]

Do you want me to open a PR with the small refactoring mentioned in #7285 (comment) ?

In truth, the more I think of it the more I think the unpacking operation seem out of place in WheelBuilder.build this method and should be moved to a higher level caller (after all unpacking is not building).

[xavfernandez]

In truth, the more I think of it the more I think the unpacking operation seem out of place in WheelBuilder.build this method and should be moved to a higher level caller (after all unpacking is not building).

👍

[chrahunt]

Yes, that's a great idea. Some hints after having looked at the code:

• If we could have standalone functions that, when called with basic required arguments (e.g. a source directory and desired output directory instead of an InstallRequirement) and returning the path to the wheel, then everything should follow from that. One for legacy and one for PEP 517.
• WheelBuilder.build could either return a list of BuildResults or be a generator for BuildResults, or we could elevate the execution to the caller.
• I would start by seeing what makes the most sense at each call site individually vs trying to satisfy both use cases at the same time.

Also smaller changes are easier, if you see some couple-line refactoring that is non-controversial and would reduce the scope of your overall change don't hesitate to submit it!