Open kaos opened 3 years ago
How would this work, technically?
As setup_py()
(or, python_artifact()
which I'd actually prefer to use, from now on) accepts any fields on it (**kwargs
) but the target python_distribution()
is not as lenient to unknown fields..
Do we add all possible fields from setup()
, or a catch all for fields "we do not care about", like python_distribution(kwargs={"some_other":"value"})
?
Adding all fields supported by setup()
feels a bit.. like a maintenance burden and unnecessary overhead. Any other ideas?
I was wondering the exact same thing. I agree that adding every additional field is unmaintainable. I think an extra_kwargs
field makes sense. When we have fields like entry_points
that warrant extra love, we can promote them to first-class: it also wouldn't be an error to keep in extra_kwargs
, just won't benefit from the special handling.
The setup_py()
plugin hook would be replaced by the much more powerful proposed target generation, which would allow you to set any of the fields for the target, not just provides=
.
But then we also hit an awkward part of the Target API: the fields need to be hashable, which is why things like DictField
use FrozenDict
. That won't flow with arbitrary types, we won't be able to coerce lists into tuples and so on because we don't know what data to expect. But, I think we can override __hash__
for the extra_kwargs
field at least..I mean, the provides
field somehow is hashable.
But I get the nagging feeling that we simply rename stuff, that way... hmm.. but maybe not so bad after all, if provides
would become extra_kwargs
or additional_kwargs
, and the PythonArtifact
type, would be a more generic Dict
type that doesn't have any requirements at all, from a users perspective (and from ours, that it is hashable), that would make it so get rid of duplication, and move all required information to top level fields, which is a win.
On a second thought on the field name for those extra args.. what it would look like when used, I think simply setup
might fit well..
python_distribution(
name="my-distro",
version="1.2.4",
setup={
"classifiers": "Tooling :: CI/CD :: Development",
}
)
# vs
python_distribution(
name="my-distro",
version="1.2.4",
extra_kwargs={
"classifiers": "Tooling :: CI/CD :: Development",
}
)
perhaps with a check on what fields are used for setup
, so it isn't one that has a top-level field already, as that wouldn't really make any sense (either it was a duplicated value, or simply missing from the python_distributions field)
Or maybe extra_setup_kwargs
?
I agree this is a big improvement over having this weird, underdocumented python_artifact
thing.
This issue is still a good idea I think, but see https://github.com/pantsbuild/pants/issues/14832 as a proposal to make objects less hacky.
still a good idea I think,
I think it's a good idea because right now we are polluting the global namespace with something specific to only python_distribution
.
--
I think the only blocker to this PR is figuring out how to __hash__
. Could we do something like call str()
on it? 👀
But then we also hit an awkward part of the Target API: the fields need to be hashable, which is why things like DictField use FrozenDict. That won't flow with arbitrary types, we won't be able to coerce lists into tuples and so on because we don't know what data to expect. But, I think we can override hash for the extra_kwargs field at least..I mean, the provides field somehow is hashable.
Personally, I would love to see setup.py just disappear; i don't want a generated setup.py either.
With my anti-setup.py
perspective, I would not want to see setup=
as a kwarg on python_distribution
.
pyproject.toml
has a [project]
section (defined by PEP 621 and https://packaging.python.org/en/latest/specifications/declaring-project-metadata/) that covers much of the metadata that setup.py normally defines. I think Setuptools is (one of?) the only backend that uses that standardized metadata.
So, I see two directions we could take to use this:
provides=python_project()
instead of provides=python_artifact(..)
, that makes pants pull the details from pyproject.toml instead inlining in the BUILD file.pyproject.toml
(or extend an existing one) with a [project]
section based on the kwargs of python_artifact
or one of the other recommendations in this issue.Or maybe some combo of modifying the pyproject.toml to add info specified in the BUILD metadata (like dependencies), and sourcing other metadata from the in-worktree copy (like the package name and version).
As commented by @jsirois, the
setup_py()
construct no longer fulfills its purpose as a standalone construct.https://github.com/pantsbuild/pants/issues/11808#issuecomment-810653736
This ticket is for deprecating
setup_py()
with new fields directly onpython_distribution()
instead.