Skip to content
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

Feature: Declare ext_modules and libraries in pyproject.toml #262

Conversation

bostonrwalker
Copy link
Contributor

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.

@sonarcloud
Copy link

sonarcloud bot commented Feb 1, 2022

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

@bostonrwalker
Copy link
Contributor Author

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

@clintonroy
Copy link

Very nice, does it handle cython extensions?

@bostonrwalker
Copy link
Contributor Author

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

@neersighted neersighted marked this pull request as draft August 23, 2022 19:33
@neersighted
Copy link
Member

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.

@bostonrwalker
Copy link
Contributor Author

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 bostonrwalker marked this pull request as ready for review August 26, 2022 13:24
@bostonrwalker bostonrwalker marked this pull request as draft August 29, 2022 11:53
@bostonrwalker bostonrwalker force-pushed the feature_extensions_defined_in_pyproject branch 4 times, most recently from bfd85f8 to 65a2f2f Compare August 29, 2022 14:17
Boston Walker and others added 8 commits August 29, 2022 10:19
Add test fixture for package with C/C++ extension modules defined entirely in pyproject.toml

Add test case for C/C++ extension defined entirely in pyproject.toml

Add new fields to pyproject.toml; Add definitions for tool.poetry.libraries and tool.poetry.ext_modules

Corrected C library build info schema

Edit descriptions to allow globs for certain properties

Add convert_libraries() and convert_ext_modules() methods; refactor source file validation

Add libraries and ext_modules to build_setup() method

Add libraries and ext_modules to package factory

Fixed glob expansion logic

Move initialization of Extension module to setup.py

Move initialization of Extension module to setup.py

Add libraries and extension modules to ProjectPackage; Add should_build_libs_and_extensions() method

Fix definition of Python module

Add library and ext_module files to find_files_to_add()

Add validation of include_dirs

Add extra_includes to find_files_to_add() to accomodate includes from C/C++/Obj-C modules; Revert validation/normalization of include_dirs in convert_libraries() and convert_ext_modules()

Process library and ext_modules includes using same mechanism as tool.poetry.include; Rename extension in test case to 'extension.extension'; Use package.should_build_libs_and_extensions() instead of package.build_script

Add case of mixed build.py and extension defined in pyproject.toml; Parameterize test

Added test case of mixed lib declaration with build.py usage

Invalid configuration when mixing build script with declared libraries/extensions

Remove redundant imports

Remove redundant import

Clean up commented lines

Clean up test case

Add simple C extension test case

Revert initialization of additional_files

Restore setup.py

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Fix glob resolution for Python <3.10

Remove comment

Remove unused import

Add custom ExtBuilder

Simplify source path resolution logic and correct treatment of globs

Fixed compat issue with Python <3.7
* factory: merge legacy dependencies and groups

This change ensures that existence of legacy "dev-dependency" section
does not break when "dev" group is added for new dependencies.

* package: remove redundant filtering

* fix typing for `ProjectPackage.python_versions`
typechecking poetry.core.masonry

Fix bugs causing tests to fail

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Add missing imoports to build.py; Use dict instead of toml.items.Table to store intermediate build info in build_ext_modules process

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Move distutils.extension.Extension import into type-checking block

Fix mypy errors

Define missing BuildFailed exception in setup.py; Rename test fixtures to avoid errors with Windows filenames being too long

Add get_export_symbols() function to ExtBuilder in setup.py for Windows compat

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Try another fix to Windows build errors

Try another fix to Windows build errors

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Remove get_export_symbols() function; Add stub functions to init C libs (for Windows compat)

Add libraries to export_symbols (for Windows)

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Rename folders to avoid path length limit when compiling/linking; Fix typos

Fix linking issues on Windows

Removed generated binary files

Revert changes to extended_no_setup test fixture

Rename ext_pyproj_lib_build_py fixture

Clarify module descriptions in README.rst files

Fix 'extended is not a package' errors when attempting to build extended_no_setup test fixture

Fix paths

Fix call to SdistBuilder.build()

Clean up rebase

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Fix exception message
@bostonrwalker bostonrwalker force-pushed the feature_extensions_defined_in_pyproject branch from 65a2f2f to 5a0a5c1 Compare August 29, 2022 14:21
@bostonrwalker bostonrwalker marked this pull request as ready for review August 29, 2022 14:28
@bostonrwalker
Copy link
Contributor Author

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 bostonrwalker changed the title Feature: Declarative approach to ext_modules and libraries Feature: Declare ext_modules and libraries in pyproject.toml Aug 29, 2022
@neersighted
Copy link
Member

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

@Secrus
Copy link
Member

Secrus commented Sep 18, 2022

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.

@bostonrwalker
Copy link
Contributor Author

bostonrwalker commented Sep 19, 2022

@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.

@sonarcloud
Copy link

sonarcloud bot commented Oct 25, 2022

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

@Secrus
Copy link
Member

Secrus commented Nov 21, 2022

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 Secrus closed this Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants