Some more trivial refactorings in WheelBuilder

[sbidoul]

Towards disentangling the pip wheel and pip install cases in WheelBuilder.

The individual commits should be easy to follow. Let me know if you prefer smaller PRs.

The third commit may require more reviewer attention, as it removes an os.path.join that is useless if I see correctly.

[xavfernandez]

This seems fine but as you pointed out, the 3rd commit is odd and even if your change seems correct, I don't how shutil.copy(wheel_file, self._wheel_dir) and shutil.copy(os.path.join(cache_dir, wheel_file), self._wheel_dir) could be equivalent.
And since the tests pass in both case, there is likely a hole in the (test) racket 😛

[sbidoul]

wheelfile and os.path.join(cache_dir, wheelfile) are actually equivalent if wheelfile is an absolute path. It apparently is absolute, because the join is done in _build_on_inside_env.

I actually noticed it because there is no equivalent os.path.join in the should_cache code path, which indicated it was either not necessary or there was a bug in the other path. So I concluded it is not necessary.

[chrahunt]

This looks good to me.

A few small things may have made it a little easier to review:

• For the extraction of _collect_buildset, it may have been clearer to set up a local need_wheel first that looks like you want, then do the extraction in a separate step.
• For the "Remove useless os.path.join in WheelBuilder" commit, maybe draw attention to why it is useless (i.e. the return value of _build_one_inside_env is already joined with the output directory). The commit message is a good place to put justification.

[xavfernandez]

wheelfile and os.path.join(cache_dir, wheelfile) are only equivalent if wheelfile is an absolute path.

Indeed, you're right ^^ Thanks for the pointer.

But this is not always true currently as options.cache_dir is not normalized :-/

[xavfernandez]

With current pip master:

$ pip wheel zbarlight --no-deps -w . --cache-dir toto
Collecting zbarlight
  Downloading zbarlight-2.3.tar.gz (6.9 kB)
Building wheels for collected packages: zbarlight
  Building wheel for zbarlight (setup.py) ... done
  Created wheel for zbarlight: filename=zbarlight-2.3-cp38-cp38-linux_x86_64.whl size=22965 sha256=f3bb202fb0490c6b6dab67a734c0b1323cf8d0301489c97c927e8e14eb43c122
  Stored in directory: toto/wheels/0f/10/fb/72368c70eeb4a795ec30116b7d9376a98a41e4be67d710e059
  WARNING: Building wheel for zbarlight failed: [Errno 2] No such file or directory: 'toto/wheels/0f/10/fb/72368c70eeb4a795ec30116b7d9376a98a41e4be67d710e059/toto/wheels/0f/10/fb/72368c70eeb4a795ec30116b7d9376a98a41e4be67d710e059/zbarlight-2.3-cp38-cp38-linux_x86_64.whl'
Failed to build zbarlight
ERROR: Failed to build one or more wheels
$ rm -rf toto/
$ pip wheel zbarlight --no-deps -w . --cache-dir /tmp/toto
Collecting zbarlight
  Downloading zbarlight-2.3.tar.gz (6.9 kB)
Building wheels for collected packages: zbarlight
  Building wheel for zbarlight (setup.py) ... done
  Created wheel for zbarlight: filename=zbarlight-2.3-cp38-cp38-linux_x86_64.whl size=22965 sha256=f7c3f161c4dc4863804c30e06e21428f569ae7c53febf85ab62e3061aa06ef79
  Stored in directory: /tmp/toto/wheels/0f/10/fb/72368c70eeb4a795ec30116b7d9376a98a41e4be67d710e059
Successfully built zbarlight

So there are definitely missing tests :)

[sbidoul]

@xavfernandez you beat me to the reproducer :) So it looks like I actually fixed a previously unknown bug?

[xavfernandez]

So it looks like I actually fixed a previously unknown bug?

It looks like it :) And found a missing test with a non-absolute --cache-dir option ^^

[sbidoul]

Hm. It might be I introduced it myself in #7285. i'll add a test for the non-absolute --cache-dir option.

[xavfernandez]

We might want to handle --cache-dir like in #7467
So an assert to check that the path is absolute might be enough here ?

[sbidoul]

I added the test for non-absolute cache directory, and reworded the commit message.

[pradyunsg]

Travis CI failed for some reason. Restarted the job.