[Distutils] PEP 517 again

[Nick Coghlan]

On 24 August 2017 at 23:11, Thomas Kluyver <thomas at kluyver.me.uk> wrote:
> Nathaniel seems to be busy with other things at the moment, so I hope he
> won't mind me passing on this list of things he'd like to resolve with
> the draft PEP. I'll quote his comments and put my responses inline.
>
>> - needs some sort of NotImplemented(Error) handling specified
>
> We've gone round return-value vs exception a few times, but I don't
> think that debate is particularly heated, so it probably just needs
> someone to say "this is what we're doing". I can be that someone if
> needs be. But does anyone feel strongly about it?

Aye, I do, and it should be "raise NotImplementedError('Explanation of
the failed check')"

Rationale:

- Python isn't C or Go, so we indicate failures with exceptions, not
error codes (NotImplemented is an necessary performance hack for
operand coercion in tight loops, not an example to be emulated in
other APIs)
- allows the backend to provide information on what went wrong
- means an unhandled backend error results in a traceback pointing to
where the build failed, not some later point in the frontend code
- if a backend developer is sufficiently worried about accidentally
propagating NotImplementedError that they want to pretend they're not
writing Python any more, they can use this idiom:

    def public_hook_api(*args, **kwds):
        try:
            result, error_msg = _internal_hook_implementation(*args, **kwds)
        except NotImplementedError as exc:
            raise RuntimeError("Unexpected NotImplementedError") from exc
        if result is NotImplemented:
            raise NotImplementedError(error_msg)
        return result

That provides the backend with all the same assurances against
accidentally letting NotImplementedError escape that a return code
based public API would, without frontends even needing to be aware of
the backend developer's aversion to reporting errors as exceptions.

>> - The out-of-tree build example (that makes an sdist and then builds
>> it) seems like exactly the kind of implementation that Donald says he
>> doesn't want to see? OTOH I think Nick said he wants to see a more
>> elaborated specification of out-of-tree build strategies with this
>> specifically as one of them.

Not really - I raised that prospect because Nathaniel was insisting
that out-of-tree builds would be too hard for backend developers to
implement, and I pointed out that they really weren't that
complicated:

- if you're wrapping a backend that supports them, pass down the
directory setting
- if you're not, implement them as semantically equivalent to build
sdist -> unpack sdist -> build wheel
- since build_sdist can fail with NotImplementedError, out-of-tree
builds are also permitted to fail that way

>> - Nick has suggested that the to-be-defined NotImplemented(Error)
>> handling can be used by build_wheel to indicate that it can't do
>> out-of-tree builds, so maybe the frontend should try an in-tree build
>> instead. I don't know how to convert this into a real spec, though --
>> like in general, if I'm a frontend and I call `hook(*args, **kwargs)`
>> and it says "can't do that", then how do I know what the problem is
>> and what I should try instead?

The fallback chains are defined by frontends, not the spec, and they depend on:

- what the frontend is trying to do
- what the fronted is trying to prevent

For the case of "build a wheel from a source tree" for example, a
reasonable fallback chain might be:

- try build_sdist
- if that raises NotImplementedError, try an explicitly out-of-tree
build_wheel call
- if that also raises NotImplementedError, try an unqualified build_wheel call

It would also be equally reasonable to skip either or both of the
first two options.

>> - What happens if prepare_build_metadata returns NotImplemented /
>> raises NotImplementedError?

Up to the frontend, but failing outright seems reasonable (since there
isn't any real reason to expect generating the entire wheel to succeed
if generating the metadata failed)

>> - I don't understand how out-of-tree builds and prepare_build_metadata
>> are supposed to interact.

They don't, since the backend should only implement
prepare_build_metadata if it can generate the metadata without
actually triggering a full build of all the binary artifacts.

>> - Also, AFAICT the out-of-tree build feature has no motivation
>> anymore. The only mentioned use case is to support incremental build
>> features in automatic build pipelines like Travis, but there are a
>> number of these build pipelines, and lots of existing build systems
>> that support out-of-tree builds, and AFAICT no-one actually uses them
>> together like this. (I think it's because most build systems'
>> out-of-tree build features are designed on the assumption that there's
>> a human running the show who will deal with any edge cases.)
>
> I believe the out-of-tree build option Nathaniel is referring to is the
> build_directory parameter to build_wheel(). His proposed APIs remove
> that parameter, and elsewhere in his email he describes that no-one
> seems to want it: everyone thinks someone else wants it.
>
> By my understanding, the reasons for including the build_directory
> parameter are:
>
> 1. Without an explicit build directory, the developers of pip are
> concerned that build backends will modify the source tree and cause
> issues which users report as bugs to pip.

This is the motivation that Donald and Paul have indicated isn't
necessarily valid any more, since they'd be OK with a setup where pip
uses the following build model by default:

1. First try the explicit build_sdist -> unpack sdist -> build_wheel path
2. If build_sdist raises NotImplementedError, fall back to trying
build_wheel directly

> 2. A frontend-controlled build directory could be used for caching
> intermediate build artifacts. This was a secondary argument after we had
> the idea, and we've never really fleshed out how we expect it to work.
> There's also a concern that if the first round of frontends always use
> an empty tempdir, backends will end up relying on that. I think this
> second argument is a weak one unless we spend the time to figure out the
> details.
>
> Do other people see the situation in a similar way? How might we move
> forwards on this?

As long as Donald & Paul are OK with it for pip, I'm OK with omitting
the build_directory parameter for now - since we're going to make
supporting it optional regardless, that means it doesn't matter as
much whether its in the initial iteration of the API or not.

>> - needs some sort of conclusion to the srcdir-on-sys.path issue.
>
> While Nathaniel is in the minority regarding srcdir-on-sys.path, he
> argues that most of us are assuming a default position without really
> thinking through it, which is certainly true of me. I don't feel
> strongly about this topic, but I do want to come to a conclusion. As
> Nathaniel does feel strongly about it, does anyone object to either:
>
> A. Leaving the source dir off sys.path when loading the hooks, and
> making a special backend which exposes hooks from the CWD.
> B. Adding another key in the TOML file to control whether hooks can be
> loaded from the source dir.

I'm mainly interested in the way things fail, and who has the power to
fix them when they break.

- when the default is "source tree is set as sys.path[0]":
  - that's essentially the same as the status quo with setup.py
  - if a project's build process has a name shadowing problem, the
publisher is going to hit that locally and fix it prior to release
- when the default is "the source tree is not set as sys.path[0]", we know that:
  - any setuptools backend is going to have to ensure that the old
default is in place when running setup.py
  - any backend that runs a Python script is going to end up with the
CWD as sys.path[0] in that script anyway due to Python's default
behaviour
  - if a project self-hosts its own build backend, we either have to
say "that's not supported", or else provide a way to change the
default

So I think this is a case where we have an established precedent (i.e.
the way setup.py currently works), and the potential for accidental
name shadowing doesn't offer a sufficiently compelling rationale for
changing that default.

Cheers,
Nick.

-- 
Nick Coghlan   |   ncoghlan at gmail.com   |   Brisbane, Australia