<div dir="ltr">> <span style="font-size:12.8px">and they do not appear to receive the source or wheel directories.</span><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">The source directory is the current directory, if I am not mistaken.</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">> </span><span style="font-size:12.8px">This lets `build_*` focus purely on building things straight away. There is a difference between "no more reqs are needed to do X" and "no possible req will achieve X" even though both add zero requirements. Why not let this hook relay it's decision more completely?</span></div><div><span style="font-size:12.8px"><br></span></div><div>Not trying to speak on behalf of flit here, but if I understand correctly, flit requires git to build a source distribution. Flit knows its build requirements, so it can just return them when get_requires is called. However, it needs to attempt to invoke git to find out whether it can build a source distribution, which if I understand correctly, is a lengthy operation (whether that's actually true is not actually relevant because we are discussion potentially any backend/operation). It's more efficient if git is only invoked once, and if a failure occurs, then under the proposal "None" would be returned from build_sdist rather than the name of the built source distribution.</div><div><br></div><div> <span style="font-size:12.8px">def get_requires_for_build_sdist(</span><wbr style="font-size:12.8px"><span style="font-size:12.8px">source_dir, ...):</span><br></div><div style="font-size:12.8px"> # I have no interest in sdists and I never will.</div><div style="font-size:12.8px"> # GO AWAY.</div><div style="font-size:12.8px"> return None</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">I have to say that the above example is not a good idea. Perhaps I was a bit muddled on that point earlier: the reason that "return None" is a good idea for build sdist is, well:</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">def build_sdist(...): # Indicates success</div><div style="font-size:12.8px"> return "x.tar.gz"</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px"><span style="font-size:12.8px">def build_sdist(...): # Indicates failure</span><br></div><div style="font-size:12.8px"><div style="font-size:12.8px"> return None</div><div><br></div><div><br></div><div>Those are not truthily equivalent, which is important because it means that someone is unlikely to make a mistake on that matter.</div></div><span class="gmail-im" style="font-size:12.8px"></span></div><div class="gmail_extra"><br><div class="gmail_quote">2017-08-26 23:05 GMT-05:00 C Anthony Risinger <span dir="ltr"><<a href="mailto:c@anthonyrisinger.com" target="_blank">c@anthonyrisinger.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Sat, Aug 26, 2017 at 9:00 PM, Nathaniel Smith <span dir="ltr"><<a href="mailto:njs@pobox.com" target="_blank">njs@pobox.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Sat, Aug 26, 2017 at 6:30 PM, C Anthony Risinger<br>
<span class="m_-9121289954404362951gmail-"><<a href="mailto:c@anthonyrisinger.com" target="_blank">c@anthonyrisinger.com</a>> wrote:<br>
> On Aug 26, 2017 5:13 PM, "Nathaniel Smith" <<a href="mailto:njs@pobox.com" target="_blank">njs@pobox.com</a>> wrote:<br>
><br>
> On Sat, Aug 26, 2017 at 1:47 PM, C Anthony Risinger<br>
> <<a href="mailto:c@anthonyrisinger.com" target="_blank">c@anthonyrisinger.com</a>> wrote:<br>
><br>
</span><span class="m_-9121289954404362951gmail-">> Sure sure, I understand all that, and why we think we need some special<br>
> error signal from `build_sdist`, as currently written.<br>
><br>
> What I'm suggesting, is maybe calling `build_sdist` without knowing if it<br>
> can succeed is already a mistake.<br>
><br>
> Consider instead, if we make the following small changes:<br>
><br>
> 1. `get_requires_for_build_*` is passed the sdist and wheel directories,<br>
> just like `build_*`, giving them the chance to actually look at tree before<br>
> deciding what other reqs might be necessary.<br>
<br>
</span>That's not a change, that's how it works :-).<br></blockquote><div><br></div></span><div>Is that a change I missed from this thread? I'm reading here:</div><div><br></div><div><a href="https://github.com/python/peps/blob/597ffba/pep-0517.txt#L301" target="_blank">https://github.com/python/<wbr>peps/blob/597ffba/pep-0517.<wbr>txt#L301</a></div><a href="https://github.com/python/peps/blob/597ffba/pep-0517.txt#L254" target="_blank">https://github.com/python/<wbr>peps/blob/597ffba/pep-0517.<wbr>txt#L254</a><div><a href="https://www.python.org/dev/peps/pep-0517/#get-requires-for-build-sdist" target="_blank">https://www.python.org/dev/<wbr>peps/pep-0517/#get-requires-<wbr>for-build-sdist</a></div><div><a href="https://www.python.org/dev/peps/pep-0517/#get-requires-for-build-wheel" target="_blank">https://www.python.org/dev/<wbr>peps/pep-0517/#get-requires-<wbr>for-build-wheel</a><br></div><div><br></div><div>and they do not appear to receive the source or wheel directories.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_-9121289954404362951gmail-">
> 2. `get_requires_for_build_*` returns None to signal `build_*` is<br>
> unsupported (superceded by static reqs defined in TOML) and [...] to signal<br>
> support (can be empty).<br>
><br>
> 3. `get_requires_for_build_*` assumed to return None if missing (so optional<br>
> and implies no support).<br>
<br>
</span>This is what I originally proposed, except you use None where I use<br>
NotImplemented, which has the disadvantages I noted earlier. Also,<br>
people didn't like the missing get_requires_for_build_* being treated<br>
as no-support, which makes sense, since we expect that<br>
get_requires_for_build_* won't be used very often. But one can switch<br>
the default here without affecting much else. The reason we want to<br>
let build_sdist report failure is just for convenience of backends who<br>
don't have any other reason to implement get_requires_for_build_sdist.<br></blockquote><div><br></div></span><div>Oh OK, good good. Well in that case I agree with you and missed the suggestion. I personally prefer NotImplemented as well here but None seemed mostly just as good and did not elicit as much pushback. It's not too big of deal either way.</div><div><br></div><div>However, a missing `get_requires_for_build_wheel` technically signaling "unsupported" makes good sense to me because it's always supplemented by the static *and mandatory* `build-system.requires` list. There is no proper way (without breaking the spec) to signal "unsupported" for `build_wheel` since the backend itself (setuptools, wheel, flit) is specified here. "Unsupported" is only signaled when *both* the static and dynamic requires are None (or NotImplemented as mentioned). The kicker here in my offering, is that the presence of `build-system.requires` *does not in any way imply* `build_sdist` support. As written, there is no way to statically set the requirements for sdist support (though this could be changed of course with a new TOML key/table), so you must explicitly signal it with something like:</div><div><br></div><div>def get_requires_for_build_sdist(*<wbr>args, **kwds): return []</div><div><br></div><div>This means, by default, `build_wheel` is "supported" and `build_sdist` is "unsupported", and both are no fail operations. If called, any exception is fatal to the entire process. If a backend goes through the work of supporting sdist creation, is it really a problem to relay this support with a 3 line function definition?</div><div><br></div><div>Wheel (could also define nothing at all):</div><div><br></div><div><div>def get_requires_for_build_sdist(<wbr>source_dir, ...):</div><div> # I have no interest in sdists and I never will.</div><div> # GO AWAY.</div><div> return None</div></div><div><br></div><div>Flit:</div><div><br></div><div><div>def get_requires_for_build_sdist(<wbr>source_dir, ...):</div><div> # I can output an sdist if the source directory is a VCS checkout I understand.</div><div> requires = get_requires_for_vcs_checkout_<wbr>or_signal_unsupported(source_<wbr>dir)</div><div> return requires</div><div><br class="m_-9121289954404362951gmail-Apple-interchange-newline">Setuptools:</div><div><br></div><div>def get_requires_for_build_sdist(<wbr>source_dir, ...):</div><div> # I'm going to successfully create an sdist or die trying!</div><div> # There is literally no directory I can't handle so I don't even look.</div><div> return []</div></div><div><br></div><div>This seems pretty straightforward to me and avoids overloading `build_sdist`, keeping it no fail like `build_wheel`.</div><div><br></div><div>Semantically, it's really the job of the requirements discovery mechanism to decide one of:</div><div><br></div><div>a) Zero or more requirements are needed to transform target source_dir into an sdist.</div><div>b) No requirement enables my backend to transform target source_dir into an sdist.</div><div><br></div><div>This lets `build_*` focus purely on building things straight away. There is a difference between "no more reqs are needed to do X" and "no possible req will achieve X" even though both add zero requirements. Why not let this hook relay it's decision more completely?</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_-9121289954404362951gmail-">
> 4. sdist reqs = `get_requires_for_build_sdist` (dynamic) + ??? (static)<br>
><br>
> 5. wheel reqs = `get_requires_for_build_wheel` (dynamic) +<br>
> `build-system.requires` (static)<br>
<br>
</span>build-system.requires contains the requirements that are always<br>
installed before we even try importing the backend, so they're<br>
available to all backend hooks equally.<br></blockquote><div><br></div></span><div>Yes absolutely. If said backend wants to also signal unconditional sdist support, it simply needs to:</div><div><br></div><div><div>def get_requires_for_build_sdist(<wbr>source_dir, ...):</div><div> return []</div></div><div><br></div><div>and call it a day. The point of my `sdist reqs =` comment was to stress that while the presence of `build-system.requires` inherently signals `build_wheel` support, it *does not* signal `build_sdist` support.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_-9121289954404362951gmail-">
> 6. If no reqs are found for sdist (no declared reqs in TOML and<br>
> `get_requires_for_build_sdist` is missing or returns None), then<br>
> `build_sdist` is unsupported.<br>
><br>
> 7. If no reqs are found for wheel (no declared reqs in TOML and<br>
> `get_requires_for_build_wheel` is missing or returns None), then<br>
> `build_wheel` is unsupported. This one is a spec violation because at least<br>
> one req is expected here (the backed itself).<br>
<br>
</span>The TOML requires aren't really useful as a signal about whether sdist<br>
specifically is supported. Plus I think we probably want to leave<br>
no-requires-in-TOML as a valid option for saying "I don't need<br>
anything installed" (maybe because the backend is shipped inside the<br>
source tree) rather than overloading it to have extra meanings.<br></blockquote><div><br></div></span><div>The spec currently states that the `requires` key is mandatory, so `build_wheel` will unconditionally signal readiness. I was more referring to if we decided to add a separate key, like `sdist-requires`, to statically signal `build_sdist` readiness via TOML. You could use such a key to pull in a plugin for the chosen build backend, such as a hypothetical `flit.sdist` package, instead of baking the support into flit.</div><div><br></div><div>Even without the addition of an `sdist-requires` TOML key, build backends can always signal sdist support dynamically by defining a basic `get_requires_for_build_sdist` returning an empty list.</div><div><br></div><div>The most basic build backend is still one that defines only `build_wheel` and nothing else. If it also defines `build_sdist`, well great, but pip and friends will not use it unless it also defines an appropriate `get_requires_for_build_sdist`<wbr>. This is an extremely low bar to me and achieves commonality in signature and behavior across all get_requires_* and build_* hooks.</div></div><span class="HOEnZb"><font color="#888888"><div><br></div>-- <br><div class="m_-9121289954404362951gmail_signature"><br>C Anthony</div>
</font></span></div></div>
<br>______________________________<wbr>_________________<br>
Distutils-SIG maillist - <a href="mailto:Distutils-SIG@python.org">Distutils-SIG@python.org</a><br>
<a href="https://mail.python.org/mailman/listinfo/distutils-sig" rel="noreferrer" target="_blank">https://mail.python.org/<wbr>mailman/listinfo/distutils-sig</a><br>
<br></blockquote></div><br></div>