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

setup: use scikit-build for building C module #424

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

jcfr
Copy link
Collaborator

@jcfr jcfr commented Jul 26, 2018

scikit-build is a drop-in replacement to setuptools.setup function
allowing to easily compile and package extensions (C/C++/Cython) by
bridging CMake and setuptools. See http://scikit-build.org

CMake is an open-source, cross-platform family of tools designed
to build, test and package software. See https://cmake.org

Ninja is a small build system with a focus on speed. It differs from other
build systems in two major respects: it is designed to have its input
files generated by a higher-level build system, and it is designed
to run builds as fast as possible. See https://ninja-build.org/

By adding the "pyproject.toml" file, we ensure the required packages
are installed when running "pip wheel . -w dist".

To streamline testing and creation of source or built distribution, the
"requirements-dev.txt" was also updated to include scikit-build, cmake
and ninja.

This commit introduces new build dependencies all available as wheels:
scikit-built, cmake and ninja.

scikit-build is a drop-in replacement to setuptools.setup function
allowing to easily compile and package extensions (C/C++/Cython) by
bridging CMake and setuptools. See http://scikit-build.org

CMake is an open-source, cross-platform family of tools designed
to build, test and package software. See https://cmake.org

Ninja is a small build system with a focus on speed. It differs from other
build systems in two major respects: it is designed to have its input
files generated by a higher-level build system, and it is designed
to run builds as fast as possible. See https://ninja-build.org/

By adding the "pyproject.toml" file, we ensure the required packages
are installed when running "pip wheel . -w dist".

To streamline testing and creation of source or built distribution, the
"requirements-dev.txt" was also updated to include scikit-build, cmake
and ninja.

This commit introduces new build dependencies all available as wheels:
scikit-built, cmake and ninja.
@amueller
Copy link
Owner

I feel like using CMake for this is a bit overkill. Like the configuration and dependencies got significantly longer. Also it requires new contributors to understand more things.

This streamlines the maintenance of the wordcloud package ensuring
an always up-to-date c module is generated.

When Python 3.8 will be release, adding an entry in each CI configuration
file will be sufficient.
@jcfr
Copy link
Collaborator Author

jcfr commented Jul 26, 2018

I anticipate, It will streamline the process:

  • no need to manually regenerate the C source
    • generating the source given the pyx across all version of pythons just worked on the CI
  • if the user want to build from source and doesn't have the compiler environment, scikit-build will give clear message about what to install and where to download it. (e.g version of visual studio, XCode, ... )

There was a lot of message in the issue tracker about which compiler, etc ... this could help. Installing, these wheels is fast and their available for all python version.

@jcfr
Copy link
Collaborator Author

jcfr commented Jul 26, 2018

Also, if this causes more problem that is is claiming to solve, I will help revert it. As you noticed, I have a good understanding of the word_cloud code base and can definitely help,

@amueller
Copy link
Owner

Alright, feel free to merge.

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.

2 participants