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

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Jan 4, 2024

We ship it with the distribution sagemath-environment.

This is needed for building the modularized distributions of the Sage library using meson-python.

With meson >= 1.4.0 (mesonbuild/meson#12674), it then suffices to set CYTHON=sage-cython.

With earlier versions of meson, one can use a native file

[binaries]
cython = 'sage-cython'

... as done in #37012 (54c95d6) and #35095.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

Comment on lines +10 to +13
# 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
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
Member 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
Member 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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 14, 2024

@tornaria Is there a dispute here in the sense of the "disputed PRs" process? https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ/m/MV6LXkqRCgAJ

@jhpalmieri
Copy link
Member

I'm posting to record a vote of -1 on behalf of Tobias Diez.

@mkoeppe mkoeppe requested a review from kiwifb March 14, 2024 23:47
@orlitzky
Copy link
Contributor

It's hard to coherently vote on all of these conflicting proposals at once. I lean towards re-adding the init files in the short term, but if the consensus is that we shouldn't do it, then I don't object to this. On the other hand, if I vote to approve this AND to re-add the init files, then we may wind up with two approved but conflicting branches.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2024

This PR here only adds a useful command and does not conflict with anything.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2024

This PR is prepared exactly how PRs should be prepared. It is not a test of ideological consistency.

It simply a useful tool for the build of where there are no __init__.py files -- even if the "re-add init files" PRs is merged. I'm using it in #35095 to build the modularized distributions using meson-python.

@orlitzky
Copy link
Contributor

It simply a useful tool for the build of where there are no __init__.py files -- even if the "re-add init files" PRs is merged. I'm using it in #35095 to build the modularized distributions using meson-python.

OK that's helpful to know.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 27, 2024

@tornaria Please check #37009 (comment)

@tornaria
Copy link
Contributor

@tornaria Please check #37009 (comment)

-1 vote from me here.

Copy link

github-actions bot commented Apr 1, 2024

Documentation preview for this PR (built with commit 1d21aa2; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mkoeppe mkoeppe requested a review from kwankyu April 7, 2024 23:16
@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 8, 2024

@tornaria Would you be able to express in technical terms what your concern about this PR is, if any?

I am using it in a follow-up for building the modularized distributions with meson-python; it has no effect on building the monolithic Sage library.

@tornaria
Copy link
Contributor

tornaria commented Apr 8, 2024

#37009 (comment)

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 8, 2024

There's nothing there, Gonzalo, just declarations.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 6, 2024

@roed314 @saraedum I think the "disputed" tag should be removed; a review has not even been started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: build disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ s: needs review v: minimal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants