poetry-core icon indicating copy to clipboard operation
poetry-core copied to clipboard

Feature: Declare ext_modules and libraries in pyproject.toml

Open bostonrwalker opened this issue 3 years ago • 11 comments

PR summary

A declarative approach to extensions using pyproject.toml. Eliminates the need for build.py in many cases.

How to use

Extension modules can now be declared in pyproject.toml:

[tool.poetry.ext_modules.mymodule] sources = ["mymodule/mymodule.c"]

The new logic will validate this config and use it to construct a distutils.extension.Extension object to be passed to distutils.setup()

Config properties

The new logic adds support for all options normally passed to distutils.extension.Extension(), and also within the libraries argument to distutils::setup(). The full list of supported config properties are:

Extension modules [tool.poetry.ext_modules]

  • sources (required) - A list of source filenames or globs
  • include_dirs - A list of directories to search for C/C++ header files
  • define_macros - A list of macros to define; each macro is defined using a 2-tuple (name, value)
  • undef_macros - A list of macros to undefine explicitly
  • library_dirs - A list of directories to search for C/C++ libraries at link time
  • libraries - A list of library names (not filenames or paths) to link against
  • runtime_library_dirs - A list of directories to search for C/C++ libraries at run time
  • extra_objects - A list of paths or globs of extra files to link with
  • extra_compile_args - Any extra platform- and compiler-specific information to use when compiling the source files
  • extra_link_args - Any extra platform- and compiler-specific information to use when linking object files together
  • export_symbols - A list of symbols to be exported from a shared extension
  • depends - A list of paths or globs of files that the extension depends on
  • language - The extension language (i.e. 'c', 'c++', 'objc')
  • optional - Boolean, specifies that a build failure in the extension should not abort the build process

C libraries [tool.poetry.libraries]

  • sources (required) - A list of source filenames or globs
  • include_dirs - A list of directories to search for C/C++ header files
  • macros - A list of macros to define; each macro is defined using a 2-tuple (name, value)

Why?

I wanted a tool that would limit everything to a single configuration file to do: dependency management, packaging and publishing.

By eliminating the need for build.py in many cases, this PR better accomplishes the stated intent of Poetry (reducing project complexity).

What if I still want to use build.py?

This doesn't stop you, it just gives you another option. If your build logic is too complex (e.g. moving files around), the declarative approach may not be sufficient.

To promote clarity and to reduce the complexity of build logic, this PR doesn't allow you to mix both approaches.

bostonrwalker avatar Jan 14 '22 17:01 bostonrwalker

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 3 Code Smells

No Coverage information No Coverage information
4.6% 4.6% Duplication

sonarqubecloud[bot] avatar Feb 01 '22 15:02 sonarqubecloud[bot]

The checks are no longer working but all tests should be passing now. It would be great if someone can review this PR.

bostonrwalker avatar Feb 01 '22 15:02 bostonrwalker

Very nice, does it handle cython extensions?

clintonroy avatar Feb 01 '22 23:02 clintonroy

Not in its current format. I would be welcome to suggestions.

bostonrwalker avatar Feb 02 '22 17:02 bostonrwalker

Hey @bostonrwalker, I'm going to go ahead and convert this to a draft for now as the history is very messy and makes review difficult. Once you've rebased this/cleaned up history and have finalized your proposed design, please mark it as ready for review.

neersighted avatar Aug 23 '22 19:08 neersighted

Hey @bostonrwalker, I'm going to go ahead and convert this to a draft for now as the history is very messy and makes review difficult. Once you've rebased this/cleaned up history and have finalized your proposed design, please mark it as ready for review.

Thanks. FYI the design has been finalized for a long time, I have only working through integrating changes for the past several months as well as pushing it through tests which are failing on Windows. As you can see that's been difficult from working on a Mac!

bostonrwalker avatar Aug 23 '22 19:08 bostonrwalker

Hey @neersighted, I'm marking the PR as ready to review now. I mucked up the history a bit - sorry for the rookie mistakes. It's my first time making such a major contribution to a public project and also using git rebase.

bostonrwalker avatar Aug 29 '22 14:08 bostonrwalker

FYI we're very interested in this, but it's meaty and none of the core team yet has bandwidth to look at it.

neersighted avatar Sep 16 '22 15:09 neersighted

While I like the idea, I am concerned with how this uses setuptools and distutils:

  • distutils module is scheduled for removal in Python 3.12, so this solution won't work in future versions of Python.
  • using setuptools within Poetry seems like a step back. Also, setuptools are not guaranteed to be present in the project environment.

Secrus avatar Sep 18 '22 21:09 Secrus

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Sep 19 '22 14:09 sonarqubecloud[bot]

@Secrus Great feedback.

While I like the idea, I am concerned with how this uses setuptools and distutils:

* `distutils` module is scheduled for removal in Python 3.12, so this solution won't work in future versions of Python.

I was able to migrate the build process from distutils to setuptools just now without issue.

* using `setuptools` within Poetry seems like a step back. Also, `setuptools` are not guaranteed to be present in the project environment.

setuptools is already used by Poetry to package source distributions. And while setuptools does not always come standard with Python, neither does a C compiler, but it seems reasonable to require both in the case that you want to build a C/C++ extension.

bostonrwalker avatar Sep 19 '22 14:09 bostonrwalker

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Oct 25 '22 14:10 sonarqubecloud[bot]

Hi @bostonrwalker. After some internal discussion, the Poetry team has decided not to proceed with the proposed feature. The most serious concerns were:

  1. Coupling to setuptools API, which (while stable) can be subject to change at some point. This would also require us to add this to documentation and that would open a question of whether we should link to setuptools docs or make our own.
  2. Introducing this feature would make it harder to change it later after people start relying on it.
  3. Amount of maintenance needed to support this feature going forward. We are already stretched quite thin, with a lot of PRs that wait because of limited maintainers' time to review them.
  4. Many extension tools in the Python world are made with setuptools script in mind (mypyc, cython for example) and it's much easier to adapt existing examples from tool documentation or stack overflow to build scripts.
  5. If more generic tools for building extensions emerge, this would block as at setuptools-like API which could, and probably would be incompatible with those tools.

Thank you for your contribution anyway. In the future, please consider discussing features with maintainers to avoid a similar situation. You can reach us on Poetry Discord or via issues/discussions in the main Poetry repo.

Secrus avatar Nov 21 '22 11:11 Secrus