[Distutils] PEP 517 again

[C Anthony Risinger]

On Sat, Aug 26, 2017 at 11:05 PM, C Anthony Risinger <c at anthonyrisinger.com>
wrote:

> On Sat, Aug 26, 2017 at 9:00 PM, Nathaniel Smith <njs at pobox.com> wrote:
>
>> On Sat, Aug 26, 2017 at 6:30 PM, C Anthony Risinger
>> <c at anthonyrisinger.com> wrote:
>> > On Aug 26, 2017 5:13 PM, "Nathaniel Smith" <njs at pobox.com> wrote:
>> >
>> > On Sat, Aug 26, 2017 at 1:47 PM, C Anthony Risinger
>> > <c at anthonyrisinger.com> wrote:
>> >
>> > 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.
>>
>> That's not a change, that's how it works :-).
>>
>
> Is that a change I missed from this thread? I'm reading here:
>
> https://github.com/python/peps/blob/597ffba/pep-0517.txt#L301
> https://github.com/python/peps/blob/597ffba/pep-0517.txt#L254
> https://www.python.org/dev/peps/pep-0517/#get-requires-for-build-sdist
> https://www.python.org/dev/peps/pep-0517/#get-requires-for-build-wheel
>
> and they do not appear to receive the source or wheel directories.
>
>
>> > 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).
>>
>> This is what I originally proposed, except you use None where I use
>> NotImplemented, which has the disadvantages I noted earlier. Also,
>> people didn't like the missing get_requires_for_build_* being treated
>> as no-support, which makes sense, since we expect that
>> get_requires_for_build_* won't be used very often. But one can switch
>> the default here without affecting much else. The reason we want to
>> let build_sdist report failure is just for convenience of backends who
>> don't have any other reason to implement get_requires_for_build_sdist.
>>
>
> 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.
>
> 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:
>
> def get_requires_for_build_sdist(*args, **kwds): return []
>
> 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?
>
> Wheel (could also define nothing at all):
>
> def get_requires_for_build_sdist(source_dir, ...):
>     # I have no interest in sdists and I never will.
>     # GO AWAY.
>     return None
>
> Flit:
>
> def get_requires_for_build_sdist(source_dir, ...):
>     # I can output an sdist if the source directory is a VCS checkout I
> understand.
>     requires = get_requires_for_vcs_checkout_or_signal_unsupported(source_
> dir)
>     return requires
>
> Setuptools:
>
> def get_requires_for_build_sdist(source_dir, ...):
>     # I'm going to successfully create an sdist or die trying!
>     # There is literally no directory I can't handle so I don't even look.
>     return []
>
> This seems pretty straightforward to me and avoids overloading
> `build_sdist`, keeping it no fail like `build_wheel`.
>
> Semantically, it's really the job of the requirements discovery mechanism
> to decide one of:
>
> a) Zero or more requirements are needed to transform target source_dir
> into an sdist.
> b) No requirement enables my backend to transform target source_dir into
> an sdist.
>
> 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?
>
>
>> > 4. sdist reqs = `get_requires_for_build_sdist` (dynamic) + ??? (static)
>> >
>> > 5. wheel reqs = `get_requires_for_build_wheel` (dynamic) +
>> > `build-system.requires` (static)
>>
>> build-system.requires contains the requirements that are always
>> installed before we even try importing the backend, so they're
>> available to all backend hooks equally.
>>
>
> Yes absolutely. If said backend wants to also signal unconditional sdist
> support, it simply needs to:
>
> def get_requires_for_build_sdist(source_dir, ...):
>     return []
>
> 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.
>
>
>> > 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).
>>
>> The TOML requires aren't really useful as a signal about whether sdist
>> specifically is supported. Plus I think we probably want to leave
>> no-requires-in-TOML as a valid option for saying "I don't need
>> anything installed" (maybe because the backend is shipped inside the
>> source tree) rather than overloading it to have extra meanings.
>>
>
> 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.
>
> 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.
>
> 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`. This is an extremely low bar to me and
> achieves commonality in signature and behavior across all get_requires_*
> and build_* hooks.
>

Also, as I want to avoid focusing too much on the prospect of introducing a
new TOML key for sdist requirements (which is likely unnecessary) flit
could just as well use `get_requires_for_build_sdist` to dynamically decide
if sdist creation is possible with something like:

def get_requires_for_build_sdist(source_dir, ...):
    try:
        import flit.sdist
    except ImportError:
        return None
    requires =
flit.sdist.get_requires_for_vcs_checkout_or_signal_unsupported(source_dir)
    return requires

This `pyproject.toml` supports sdists:

[build-system]
requires = ["flit", "flit.sdist"]

This `pyproject.toml` only supports wheels:

[build-system]
requires = ["flit"]

-- 

C Anthony
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/distutils-sig/attachments/20170826/6de00df3/attachment.html>