On Sat, Aug 26, 2017 at 11:05 PM, C Anthony Risinger <c...@anthonyrisinger.com> wrote:
> On Sat, Aug 26, 2017 at 9:00 PM, Nathaniel Smith <n...@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" <n...@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#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
_______________________________________________ Distutils-SIG maillist - Distutils-SIG@python.org https://mail.python.org/mailman/listinfo/distutils-sig
