-
-
Notifications
You must be signed in to change notification settings - Fork 402
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build: respect the "pure" argument at metadata generation time #1417
Conversation
When a pure build is requested, no compiled extensions are produced. The setuptools_rust dependency ends up imported-but-not-used. Teach setuptools to avoid build-requiring it in this case. Closes: jelmer#1405
434a5d4
to
9fea5e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat! I stand corrected - thanks for the fix.
else: | ||
setup_requires = ["setuptools_rust"] | ||
# We check for egg_info since that indicates we are running prepare_metadata_for_build_* | ||
if "egg_info" in sys.argv: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how future-proof is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is heavily baked into the setuptools design so if it ever did change I would expect it to be extremely long-term... and there are a bunch of packages that work like this already.
@jaraco / @abravalheri could confirm if there's any plans to change it -- but I think it would break far too many projects to be worth changing, especially if no replacement is available for the existing comment:
# Ideally, setuptools would just provide a way to do this
Either setuptools needs to provide a full-fledged API for defining build options, or it needs to allow people to execute build options in setup.py as pure python code hooking into phases such as egg_info
and conditionally defining build metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setuptools is driving users toward standards-based tooling. Setuptools' PEP 517 implementation passes setup_requires
through as get_requires_for_build_*
. However, I'd only expect this setting to have effect when building the sdist or the wheel. I'd not expect it to occur when running egg-info
.
Regardless, Setuptools has deprecated use of setup.py
invocations, so if this behavior is needed, I'd work to investigate alternative reasons to support setup.py egg_info
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't invoking setup.py here, the expected use of dulwich is to run python -m build
and as of today, this PR works fine.
When python -m build
runs the official get_requires_for_build_*
hook provided by setuptools.build_meta
, setuptools internally runs egg_info, that is why we check sys.argv
-- because we only want to import the setuptools_rust package that is used for defining Rust extensions, when running build_wheel
, not when running get_requires_for_*
(setup.py will error out if the dependencies aren't installed).
It is necessary because setup.py is being used to execute code as part of setuptools.build_meta
-- no one is running python setup.py ....
at any point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be interested to hear if there's a better way to tell, when using setup.py as a configuration file for:
- configuring extension modules
- defining additional metadata for
get_requires_for_build_*
which PEP 517 hook is currently running.
My understanding is that currently this means checking sys.argv
. If it contains "egg_info" then the PEP 517 hook for get_requires_for_build_*
is running, but if it contains "build" or "build_py" or "build_ext" then one of those 3 setuptools.setup(cmdclass={...}
are running (possibly overridden but in this case we don't override them).
When a pure build is requested, no compiled extensions are produced. The setuptools_rust dependency ends up imported-but-not-used. Teach setuptools to avoid build-requiring it in this case.
Closes: #1405