On Aug 26, 2017 5:13 PM, "Nathaniel Smith" <njs@pobox.com> wrote:
On Sat, Aug 26, 2017 at 1:47 PM, C Anthony Risinger
<c@anthonyrisinger.com> wrote:
> On Aug 26, 2017 2:17 PM, "Nathaniel Smith" <njs@pobox.com> wrote:
>>
>> [removed Guido from CC]
>>
>> On Aug 26, 2017 02:29, "Paul Moore" <p.f.moore@gmail.com> wrote:
>>
>> On 26 August 2017 at 03:17, Guido van Rossum <guido@python.org> wrote:
>> > In pretty much any other context, if you have an operation that returns
>> > an
>> > regular value or an error value, the error value should be None.
>> > (Exceptions
>> > include e.g. returning a non-negative int or -1 for errors, or True for
>> > success and False for errors.)
>>
>> So, given that build_sdist returns the path of the newly built sdist,
>> the correct way to signal "I didn't manage to build a sdist" would be
>> to return None.
>>
>> Now that it's put this way, it seems glaringly obvious to me that this
>> is the correct thing to do.
>>
>>
>> Eh... I would really prefer something that's (a) more explicit about what
>> specifically went wrong, and (b) harder to return by accident. It's not at
>> all obvious that if the list of requirements is 'None' that means 'this
>> build supports making sdists in general but cannot make them from this
>> source tree but might still be able to make a wheel'. And if you forget to
>> put in a return statement, then python returns None for you, which seems
>> like it could lead to some super confusing error modes.
>
>
> Why does the frontend need to know why an sdist was not created?

This whole discussion is about handling a specific case: suppose you
have a frontend like pip that when given a source directory and asked
to build a wheel, wants to implement that as:
  - build sdist
  - unpack sdist
  - build wheel from unpacked sdist

And suppose you have a backend like flit, that can build sdists from
some source directories (e.g. VCS checkouts) but not others (e.g.
unpacked sdists). We need some way for pip and flit to negotiate that
even though pip *normally* would implement its build-a-wheel operation
by first building an sdist, in this case it's ok to silently fall back
to some other strategy (like building the wheel directly in the source
tree, or manually copying the source tree somewhere else and then
building a wheel in it).

But, we don't want this fallback behavior to hide real bugs. So if the
backend says "look, I just can't do sdists here, and that's an
expected thing, it's not something where the user needs to take any
particular action like filing a bug report or fixing their system or
anything like that, so if you have an alternative way to accomplish
what you're trying to do then you should just silently discard this
error and try that", ...cool. But if it doesn't explicitly say that,
then we don't want to silently discard the error and do something
else.

It's taken a *lot* of back and forth to reach consensus that all we
need here is some special error signal from the *_sdist operations.
Let's focus on resolving that :-)
Sure sure, I understand all that, and why we think we need some special error signal from `build_sdist`, as currently written.

What I'm suggesting, is maybe calling `build_sdist` without knowing if it can succeed is already a mistake.

Consider instead, if we make the following small changes:

1. `get_requires_for_build_*` is passed the sdist and wheel directories, just like `build_*`, giving them the chance to actually look at tree before deciding what other reqs might be necessary.

2. `get_requires_for_build_*` returns None to signal `build_*` is unsupported (superceded by static reqs defined in TOML) and [...] to signal support (can be empty).

3. `get_requires_for_build_*` assumed to return None if missing (so optional and implies no support).

4. sdist reqs = `get_requires_for_build_sdist` (dynamic) + ??? (static)

5. wheel reqs = `get_requires_for_build_wheel` (dynamic) + `build-system.requires` (static)

6. If no reqs are found for sdist (no declared reqs in TOML and `get_requires_for_build_sdist` is missing or returns None), then `build_sdist` is unsupported.

7. If no reqs are found for wheel (no declared reqs in TOML and `get_requires_for_build_wheel` is missing or returns None), then `build_wheel` is unsupported. This one is a spec violation because at least one req is expected here (the backed itself).

This arrangement allows pip to know ahead-of-time if `build_sdist` is appropriate. If no sdist reqs are explicitly acknowledged, then no sdist can be created. Full stop.

Example usages:

* Backeds that only support wheel creation do not implement `get_requires_for_build_sdist` at all.

* Backeds that conditionally support sdist creation implement `get_requires_for_build_sdist` and return None if unsupported. This is where flit signals "impossible" from an unpacked sdist and [...] from VCS.

* Backeds that always support sdist creation (setuptools) implement `get_requires_for_build_sdist` and return [...] unconditionally.

So for pip's sdist -> unpack -> wheel path, it knows right away if it should build an sdist or skip right to building a wheel because the backend will inform via `get_requires_for_build_sdist` instead of overloading `build_sdist`.

I also think this achieves nice parity across sdist and wheel operations, keeps the same hooks optional, adds no special cases, and maybe even less overhead.

What say you?

-- 

C Anthony