[Distutils] PEP 517 again

[C Anthony Risinger]

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.

-- 

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