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.

Sign up for GitHub

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

src/bin/sage-cython: Repurpose as PEP 420 fixer #37009

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
+21 −31
Open

src/bin/sage-cython: Repurpose as PEP 420 fixer #37009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkgs/sagemath-environment/pyproject.toml.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ script-files = [
"bin/sage-version.sh",
# Auxiliary script for invoking Python in the Sage environment
"bin/sage-python",
# The following is freshly un-deprecated as a temporary workaround
# for defects in Cython 3.0.x support for PEP 420 implicit namespace packages
"bin/sage-cython",
# Not included:
# - bin/sage-env-config -- installed by sage_conf
# - bin/sage-env-config.in -- not to be installed
Expand Down
49 changes: 18 additions & 31 deletions src/bin/sage-cython
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,26 @@
#!/usr/bin/env sage-python
#!/usr/bin/env python

# This script is a deprecated wrapper around the "cython" program.
# It is deprecated since Issue #27041 (Sage 8.7) because one should
# This script is a wrapper around the "cython" program.
# It was deprecated in Issue #27041 (Sage 8.7) because one should
# simply use "cython" directly. We display deprecation messages whenever
# "sage-cython" does something different from plain "cython".
#
# A stronger deprecation warning was added in #29923 (Sage 9.2).
#
# In Sage 10.3, the deprecated functionality is removed permanently.
# However, the script is un-deprecated and re-purposed as a temporary workaround
# for defects in Cython 3.0.x support for PEP 420 implicit namespace packages,
# see https://github.com/sagemath/sage/pull/36228
#
Comment on lines +10 to +13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't un-deprecate this. Instead it should be removed in favor of using an unpatched version of cython.

Right now, compiling cython files that use sage modules is broken because of namespace packages, and patching cython here and in sage_setup gives the misleading impression that it works.

This is the opposite of "fitting in the python/cython ecosystem".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that it is a "temporary workaround".

Anyone report the namespace problem to Cython upstream yet?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temporary hack. And when/if cython fixes it we still have to ship the hack since distros are slow updating cython, etc.

Let's stick to stable features, not the bleeding edge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tornaria Kindly limit the use of fighting words such as "hack".

Let's stick to stable features, not the bleeding edge.

We have been successfully using Cython with namespace packages ever since 2021.

If anything, it is that our porting effort to Cython 3 is incomplete.

# This script can be used as a replacement for "cython".
# For example, to have meson use it, set the environment variable
# CYTHON to sage-cython./
# https://github.com/mesonbuild/meson/blob/e4bbc630b67ef97ad842badd00855e64cff12e13/mesonbuild/envconfig.py#L91

import os
import sys
args = sys.argv[1:]

sys.stderr.write("WARNING: the script sage-cython is deprecated; use cython instead.\n")

from sage.env import SAGE_SRC

# args can have length 0, in case we're printing a usage message (see trac 12207)
pyx_file = os.path.abspath(args[-1]) if len(args) else None
include_sage_flags = False

if pyx_file and pyx_file.startswith(SAGE_SRC):
sys.stderr.write("WARNING: in the future, sage --cython will not add special flags for files in Sage sources.\n")
include_sage_flags = True

if '-sage' in args:
sys.stderr.write("WARNING: sage --cython -sage is deprecated.\n")
include_sage_flags = True
args.remove('-sage')
elif '-no-sage' in args:
sys.stderr.write("WARNING: sage --cython -no-sage is deprecated, remove the -no-sage flag.\n")
include_sage_flags = False
args.remove('-no-sage')
from sage.misc.package_dir import cython_namespace_package_support

if include_sage_flags:
args = ['--embed-positions', '-I%s' % SAGE_SRC] + args
# console scripts declared in https://github.com/cython/cython/blob/master/setup.py#L68
from Cython.Compiler.Main import setuptools_main

args.insert(0, 'sage-cython')
os.execlp('cython', *args)
with cython_namespace_package_support():
setuptools_main()
Loading