tulip-control / dd

Binary Decision Diagrams (BDDs) in pure Python and Cython wrappers of CUDD, Sylvan, and BuDDy
https://pypi.org/project/dd
Other
181 stars 39 forks source link

ensure source distribution is independent of optional dependencies #36

Closed johnyf closed 6 years ago

johnyf commented 6 years ago

92543050965fc602ed987615c3469c22faa11486 introduced use of cysignals to allow Ctrl+C interruption of CUDD calls. Cython's conditional compilation was used to cimport the cysignals package or not. If cysignals is present (i.e., already installed) when cythonizing dd/cudd.pyx, then dd/cudd.c and thus the module dd.cudd will depend on cysignals.

Windows is unsupported as of cysignals >= 1.7.0. So cysignals is an optional dependency of dd:

https://github.com/johnyf/dd/blob/dc0b3c3a178064a3eeb1d2a8245bc6635c52860f/setup.py#L48-L49

https://github.com/johnyf/dd/blob/dc0b3c3a178064a3eeb1d2a8245bc6635c52860f/requirements.txt#L2

Wheel and source distributions present a challenge in this area.

Wheels

If cysignals is present when creating a wheel file (python setup.py bdist_wheel), then dd.cudd will depend on cysignals. Wheels with compiled dd.cudd are platform specific, so in principle this dependence can always be satisfied on that specific platform (otherwise cysignals would have been absent when building the wheel file). However, cysignals is absent from install_requires. So the resulting wheel file will be installed with missing requirements.

A solution is to use an environment marker for conditional dependency specification (PEP 508, see setuptools docs). For example:

install_requires=[
    'cysignals >= 1.7.0; platform_system = "Linux" or platform_system = "Darwin"',
    ...
    ]

In theory, this would work, adding the cysignals to the requirements for Linux and Darwin wheels.

Source archives

What about the source distribution? As recommended by Cython's docs, we include both cudd.pyx (source) and cudd.c (generated by cythonize) in the source distribution, in order to allow compiling dd.cudd in absence of Cython (at least in principle).

If cysignals happens to be installed in the environment of python setup.py sdist, then the generated cudd.c depends on cysignals (headers due to cimport). So compiling in absence of cysignals will raise an error, even though the environment marker platform_system (discussed above) would ensure that cysignals is present where possible.

In other words, Cython's conditionals are interpreted "early", when cythonize runs. If that happens on a platform different than the target one, then cysignals is injected as a dependency when it shouldn't (contrast to an #ifdef).

Moreover, there should be one source distribution, not multiple (unlike wheels, which can be platform-specific). So dd/cudd.c should be independent of cysignals.

With the current implementation:

https://github.com/johnyf/dd/blob/dc0b3c3a178064a3eeb1d2a8245bc6635c52860f/download.py#L86-L87

we need to uninstall cysignals before running python setup.py sdist --cudd (and clean dd/cudd.c, if it already exists; an additional complication). It is easy to forget this step. Asserting that:

assert cysignals is None or 'sdist' not in args  # where `args` those passed to `setup.py sdist`

would avoid forgetting. A smoother alternative is to set HAVE_CYSIGNALS = False if building a source distribution (in that case we should rename the directive to USE_CYSIGNALS).

Conclusion

In principle, all this would work. Simple is better than complex [PEP 20]. Indeed, if install_requires contains cysignals, then python setup.py install --cudd raises an error:

error: Setup script existed with error: The package cysignals will not function correctly when built as egg. Therefore, it cannot be installed using 'python setup.py install' or 'easy_install'. Instead, use 'pip install' to install cysignals.

However, we cannot install dd.cudd from source using pip (via pip only from wheels), due to how --install-option propagates. Even if we could, python setup.py install should remain possible.

Thus, cysignals cannot be included in install_requires, so neither source nor wheel distributions should depend on cysignals. To ensure this, a suitable assertion is needed (as above, but also for bdist), and cleaning before creating the distribution. Adapting the following Makefile rules seems appropriate:

https://github.com/johnyf/dd/blob/dc0b3c3a178064a3eeb1d2a8245bc6635c52860f/Makefile#L15-L17

https://github.com/johnyf/dd/blob/dc0b3c3a178064a3eeb1d2a8245bc6635c52860f/Makefile#L36-L44

To use cysignals, one would pip install cysignals cython (or pip install -r requirements.txt), and then python setup.py install --cudd.

johnyf commented 6 years ago

Ctrl + \ quits a process running Cython code, on both Linux and Darwin (by sending the signal SIGQUIT). See stty -a.

In contrast, Ctrl + C sends the signal SIGINT, which Cython ignores by default.

johnyf commented 6 years ago

Addressed in e391daf706153056aabe9b1841c5206beedfb516, eaf804b1717c401c5df84f842540f4c7178cdc4d.