<div dir="auto"><div><div class="gmail_extra"><div style="font-family:sans-serif" dir="auto"><div data-smartmail="gmail_signature">On Aug 26, 2017 5:13 PM, "Nathaniel Smith" <<a href="mailto:njs@pobox.com">njs@pobox.com</a>> wrote:</div><div><div class="elided-text"><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>On Sat, Aug 26, 2017 at 1:47 PM, C Anthony Risinger<br><<a href="mailto:c@anthonyrisinger.com">c@anthonyrisinger.com</a>> wrote:<br>> On Aug 26, 2017 2:17 PM, "Nathaniel Smith" <<a href="mailto:njs@pobox.com">njs@pobox.com</a>> wrote:<br>>><br>>> [removed Guido from CC]<br>>><br>>> On Aug 26, 2017 02:29, "Paul Moore" <<a href="mailto:p.f.moore@gmail.com">p.f.moore@gmail.com</a>> wrote:<br>>><br>>> On 26 August 2017 at 03:17, Guido van Rossum <<a href="mailto:guido@python.org">guido@python.org</a>> wrote:<br>>> > In pretty much any other context, if you have an operation that returns<br>>> > an<br>>> > regular value or an error value, the error value should be None.<br>>> > (Exceptions<br>>> > include e.g. returning a non-negative int or -1 for errors, or True for<br>>> > success and False for errors.)<br>>><br>>> So, given that build_sdist returns the path of the newly built sdist,<br>>> the correct way to signal "I didn't manage to build a sdist" would be<br>>> to return None.<br>>><br>>> Now that it's put this way, it seems glaringly obvious to me that this<br>>> is the correct thing to do.<br>>><br>>><br>>> Eh... I would really prefer something that's (a) more explicit about what<br>>> specifically went wrong, and (b) harder to return by accident. It's not at<br>>> all obvious that if the list of requirements is 'None' that means 'this<br>>> build supports making sdists in general but cannot make them from this<br>>> source tree but might still be able to make a wheel'. And if you forget to<br>>> put in a return statement, then python returns None for you, which seems<br>>> like it could lead to some super confusing error modes.<br>><br>><br>> Why does the frontend need to know why an sdist was not created?<br><br></div>This whole discussion is about handling a specific case: suppose you<br>have a frontend like pip that when given a source directory and asked<br>to build a wheel, wants to implement that as:<br>  - build sdist<br>  - unpack sdist<br>  - build wheel from unpacked sdist<br><br>And suppose you have a backend like flit, that can build sdists from<br>some source directories (e.g. VCS checkouts) but not others (e.g.<br>unpacked sdists). We need some way for pip and flit to negotiate that<br>even though pip *normally* would implement its build-a-wheel operation<br>by first building an sdist, in this case it's ok to silently fall back<br>to some other strategy (like building the wheel directly in the source<br>tree, or manually copying the source tree somewhere else and then<br>building a wheel in it).<br><br>But, we don't want this fallback behavior to hide real bugs. So if the<br>backend says "look, I just can't do sdists here, and that's an<br>expected thing, it's not something where the user needs to take any<br>particular action like filing a bug report or fixing their system or<br>anything like that, so if you have an alternative way to accomplish<br>what you're trying to do then you should just silently discard this<br>error and try that", ...cool. But if it doesn't explicitly say that,<br>then we don't want to silently discard the error and do something<br>else.<br><br>It's taken a *lot* of back and forth to reach consensus that all we<br>need here is some special error signal from the *_sdist operations.<br>Let's focus on resolving that :-)<br></blockquote></div></div></div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif">Sure sure, I understand all that, and why we think we need some special error signal from `build_sdist`, as currently written.</div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif">What I'm suggesting, is maybe calling `build_sdist` without knowing if it can succeed is already a mistake.</div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif">Consider instead, if we make the following small changes:</div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif">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.</div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif">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).</div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif">3. `get_requires_for_build_*` assumed to return None if missing (so optional and implies no support).</div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif">4. sdist reqs = `get_requires_for_build_sdist` (dynamic) + ??? (static)<br></div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif">5. wheel reqs = `get_requires_for_build_wheel` (dynamic) + `build-system.requires` (static)</div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif">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.</div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif"><div dir="auto">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).</div><br></div><div dir="auto" style="font-family:sans-serif">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.</div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif">Example usages:</div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif">* Backeds that only support wheel creation do not implement `get_requires_for_build_sdist` at all.</div><div dir="auto" style="font-family:sans-serif"><br>* 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.</div><div dir="auto" style="font-family:sans-serif"><br>* Backeds that always support sdist creation (setuptools) implement `get_requires_for_build_sdist` and return [...] unconditionally.<br></div><div dir="auto" style="font-family:sans-serif"><br></div><div dir="auto" style="font-family:sans-serif"><font face="sans-serif">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`.</font></div><div dir="auto" style="font-family:sans-serif"><font face="sans-serif"><br></font></div><div dir="auto" style="font-family:sans-serif"><font face="sans-serif">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.</font></div><div dir="auto" style="font-family:sans-serif"><font face="sans-serif"><br></font></div><div dir="auto" style="font-family:sans-serif"><font face="sans-serif">What say you?</font></div><div dir="auto" style="font-family:sans-serif"><font face="sans-serif"><br></font></div><div dir="auto" style="font-family:sans-serif"><font face="sans-serif">-- </font></div><div dir="auto" style="font-family:sans-serif"><font face="sans-serif"><br></font></div><div dir="auto" style="font-family:sans-serif"><font face="sans-serif">C Anthony</font></div></div></div></div>