> and they do not appear to receive the source or wheel directories.

The source directory is the current directory, if I am not mistaken.

> 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?

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.

 def get_requires_for_build_sdist(source_dir, ...):
    # I have no interest in sdists and I never will.
    # GO AWAY.
    return None

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:

def build_sdist(...):  # Indicates success
    return "x.tar.gz"

def build_sdist(...):  # Indicates failure
    return None


Those are not truthily equivalent, which is important because it means that someone is unlikely to make a mistake on that matter.

2017-08-26 23:05 GMT-05:00 C Anthony Risinger <c@anthonyrisinger.com>:
On Sat, Aug 26, 2017 at 9:00 PM, Nathaniel Smith <njs@pobox.com> wrote:
On Sat, Aug 26, 2017 at 6:30 PM, C Anthony Risinger
<c@anthonyrisinger.com> wrote:
> 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:
>
> 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#L254

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

_______________________________________________
Distutils-SIG maillist  -  Distutils-SIG@python.org
https://mail.python.org/mailman/listinfo/distutils-sig