Clarify WheelBuilder.build() a bit

[sbidoul]

This follows #6853 (comment)

cc/ @cjerdonek

[pradyunsg]

Are we sure the wheel cache is available at all call sites?

[sbidoul]

@pradyunsg goog catch. A unit test caught it too. Fixed.

[cjerdonek]

Since this was working before without first checking that self.wheel_cache is truthy, does this mean we can add the following to the above asserts?

if autobuilding:
    assert self.wheel_cache

(or add and self.wheel_cache to the first expression of your existing assert)

PS - here is where install sets wheel_cache (unconditionally):

pip/src/pip/_internal/commands/install.py

Line 318 in 76a8954

wheel_cache = WheelCache(options.cache_dir, options.format_control)

PPS - wheel also seems to set it.

[sbidoul]

The failing test was due to this mock providing no wheelcache:

pip/tests/unit/test_wheel.py

Line 700 in 76a8954

finder=Mock(), preparer=Mock(), wheel_cache=None,

[sbidoul]

So I fixed the test instead.

[cjerdonek]

Thanks for filing the PR! A couple minor comments.

[cjerdonek]

Rather than making this change, how about using this PR also as an opportunity to rename autobuilding to something more self-descriptive like should_unpack (as I suggested here). That way the argument better describes what it does and will better match the semantics of what we want going forward. (And judging by the docstring, it looks like the original author was thinking about using something like that as the name, too, anyways.) The word "auto-building" still seems confusing to me. :) Also, I'm suggesting should_unpack rather than e.g. unpack because it will be a unique string in the code base, making it easy to search.

[cjerdonek]

Also, maybe the argument description can be clarified to say "If True, after building the wheel, unpack it and replace the sdist with the unpacked version in preparation for installation." That way it's clear that not just the replacement but also the unpacking only happens if the argument is true.

[sbidoul]

Done

[cjerdonek]

Now that you've changed the test to fix all call sites, how about adding assert self.wheel_cache to the beginning?

[sbidoul]

I did not do it so far because I figured it was obvious enough that wheel_cache was a mandatory argument of the WheelBuilder constructor (per the typing declaration), and in case one makes the error, the crash reason will be as easy to understand as an assert. So I thought an additional assert about that would have little additional information value.

[cjerdonek]

So I thought an additional assert about that would have little additional information value.

I wouldn't say it adds little value as I do think it helps people reading the code. (Case in point: it would have answered @pradyunsg's question.) But I agree it's redundant with the type annotation, so I'm okay with leaving it out.

[sbidoul]

I also added a couple of clarification comments in should_use_ephemeral_cache.

Also I'm thinking that, inside should_use_ephemeral_cache, not should_unpack would be better named must_build, in slight anticipation of what we'll do next.

[cjerdonek]

Great!

[cjerdonek]

Also I'm thinking that, inside should_use_ephemeral_cache, not should_unpack would be better named must_build, in slight anticipation of what we'll do next.

Were you saying you wanted to do this now, or were you referencing what you wanted to do in the other PR? As that change is limited to just one function, maybe it would be okay to do it as part of the other PR (and since I'd be okay merging this one now).

[sbidoul]

Were you saying you wanted to do this now, or were you referencing what you wanted to do in the other PR?

I had no preference about that. It was more a mention to hint at what I have in mind for the other PR and see if you had an opinion on that name.

[sbidoul]

I squashed the commits. Ready to merge.

[cjerdonek]

see if you had an opinion on that name.

Yeah, that seems like a good name. 👍

[cjerdonek]

Thanks again! 👍

[cjerdonek]

Also I'm thinking that, inside should_use_ephemeral_cache, not should_unpack would be better named must_build, in slight anticipation of what we'll do next.

Maybe build() should also grow an additional argument like that (e.g. build_optional or force_build) since it's orthogonal to whether unpacking should take place. It just so happens that they coincide (or don't coincide), depending on how you look at it.