Building --without-libyaml doesn't work

aplaice commented 1 year ago

To reproduce

(Ensure that cython and libyaml are installed. Otherwise, the build will be without libyaml irrespective of CLI args. Optionally set up a virtualenv to avoid polluting your global python installation.)
Run python setup.py --without-libyaml install.
Run python -c 'import yaml; print(yaml.__with_libyaml__)'.

Expected result

YAML is built/installed without the yaml C extension. (2 returns False.)

Actual result

YAML is built/installed with the yaml C extension. (2 returns True.)

Background

I'm co-maintaining an Anki add-on (CrowdAnki) of which pyyaml is a dependency. Unfortunately, due to the way the Anki ecosystem and the official distribution of add-ons (via AnkiWeb) works, the add-ons have to be distributed along with all their dependencies, and the bundles have to be pure python. (AFAIU this AnkiWeb limitation is partially for security reasons and partially for convenience, so that the same builds works on all systems.) Hence, I need to ensure that the pyyaml install is without libyaml. (For my own purposes, I could simply ensure that I don't have libyaml installed (apt-get remove libyaml-dev :)), but it's not a great solution (globally uninstalling a library) and it becomes poorly-scalable when one takes into account other contributors, CI etc.)

(The underlying need is having a means of installing pyyaml without extensions.)

Causes

AFAICT the issue is caused by this line:

            with_ext = getattr(self, ext.attr_name) or os.environ.get('PYYAML_FORCE_{0}'.format(ext.feature_name.upper()))

with_ext seems to have the semantics where:

None means "non-disabled but optional"
0 means "disabled"
1 (or anything else) means "non-disabled and non-optional"

self.with_libyaml ("self=Distribution") is correctly set, where (--with-libyaml results in 1, --without-libyaml results in 0 and the absence of the flag means None. Unfortunately, when the relevant environment variable is absent, os.environ.get('PYYAML_FORCE_{0}'.format(ext.feature_name.upper())) returns None, and 0 or None is None, so we end up with "non-disabled but optional".

Something like:

            with_ext = getattr(self, ext.attr_name)
            if with_ext is None:
                with_ext = os.environ.get('PYYAML_FORCE_{0}'.format(ext.feature_name.upper()))

seems to resolve the issue (I can open a PR if this makes sense).

Running PYYAML_FORCE_LIBYAML=0 python setup.py install does actually force a built without the yaml extension (because None or '0' == '0'), but it's not intuitive and not (currently) documented.

nitzmahone commented 1 year ago

All that ancient distutils stuff is getting blown away Real Soon Now anyway, since it's already gone from stdlib in 3.12, deprecated under setuptools' vendored copy, and not supported at all by the upcoming Cython 3.0. So we're going to be completely repaving the extension build as PEP517-native, probably with an in-tree build backend to be able to accommodate the extensionless builds...

Basically, I wouldn't rely on anything that's still calling setup.py even today- just use a PEP517 builder of your choice and set the envvars. Those will likely continue to work as-is on $new_thing, where the custom CLI stuff is all going away. setuptools and pip finally have a fully working --config-settings passthru that we might also use, but that's still TBD once we get deeply into the packaging repave.

Since you've already got a workaround, I'm going to close this as a "won't fix", since it'll be moot soon anyway.

nitzmahone commented 1 year ago

Another thing that's holding up the extension build/packaging repave is trying to decide how to accommodate, eg, a future libfyaml extension option, a Cythonized version of the pure-Python code, and other possibilities. We could go the way ruamel and others have and turn the extension into an explicit child package, but as we've already experienced, even if it's really tightly coupled, it introduces all sorts of new failure modes to account for with real-world distro and user PYTHONPATH stuff.

aplaice commented 1 year ago

That makes sense! Thanks very much for the background!

yaml / pyyaml