sagemath / sage

Main repository of SageMath. Now open for Issues and Pull Requests.
https://www.sagemath.org
Other
1.08k stars 394 forks source link

build: show all cython warnings #37885

Open tornaria opened 2 weeks ago

tornaria commented 2 weeks ago

Motivated by https://github.com/sagemath/sage/pull/37792#issuecomment-2053663025, change cython configuration so we show all warnings.

This will lead to a number of warnings, but it's IMO better to know about that and works towards fixing as much warnings as possible (and we have a fair number of warnings that are indeed relevant and not just false positives).

:memo: Checklist

mkoeppe commented 2 weeks ago

This change won't take effect for most people because the editable build (via src/setup.py) does not go through sage_build_cython

github-actions[bot] commented 2 weeks ago

Documentation preview for this PR (built with commit 4dbb878e4bfe83ed43879e919931850e78656788; changes) is ready! :tada: This preview will update shortly after each push to this PR.

tornaria commented 2 weeks ago

What is going on here:

  [sagemath_categories-10.4.beta4] [spkg-check] **********************************************************************
  [sagemath_categories-10.4.beta4] [spkg-check] File "sage/arith/power.pyx", line 49, in sage.arith.power.generic_power
  [sagemath_categories-10.4.beta4] [spkg-check] Failed example:
  [sagemath_categories-10.4.beta4] [spkg-check]     F = Zmod(5)
  [sagemath_categories-10.4.beta4] [spkg-check] Exception raised:
  [sagemath_categories-10.4.beta4] [spkg-check]     Traceback (most recent call last):
  [sagemath_categories-10.4.beta4] [spkg-check]       File "/sage/pkgs/sagemath-categories/.tox/sagepython-sagewheels-nopypi-norequirements/lib/python3.10/site-packages/sage/doctest/forker.py", line 714, in _run
  [sagemath_categories-10.4.beta4] [spkg-check]         self.compile_and_execute(example, compiler, test.globs)
  [sagemath_categories-10.4.beta4] [spkg-check]       File "/sage/pkgs/sagemath-categories/.tox/sagepython-sagewheels-nopypi-norequirements/lib/python3.10/site-packages/sage/doctest/forker.py", line 1146, in compile_and_execute
  [sagemath_categories-10.4.beta4] [spkg-check]         exec(compiled, globs)
  [sagemath_categories-10.4.beta4] [spkg-check]       File "<doctest sage.arith.power.generic_power[6]>", line 1, in <module>
  [sagemath_categories-10.4.beta4] [spkg-check]         F = Zmod(Integer(5))
  [sagemath_categories-10.4.beta4] [spkg-check]     NameError: name 'Zmod' is not defined
  [sagemath_categories-10.4.beta4] [spkg-check] **********************************************************************
  [sagemath_categories-10.4.beta4] [spkg-check] **********************************************************************
  [sagemath_categories-10.4.beta4] [spkg-check] File "sage/categories/additive_monoids.py", line 41, in sage.categories.additive_monoids.AdditiveMonoids
  [sagemath_categories-10.4.beta4] [spkg-check] Failed example:
  [sagemath_categories-10.4.beta4] [spkg-check]     C.Algebras(QQ).is_subcategory(AlgebrasWithBasis(QQ))
  [sagemath_categories-10.4.beta4] [spkg-check] Exception raised:
  [sagemath_categories-10.4.beta4] [spkg-check]     Traceback (most recent call last):
  [sagemath_categories-10.4.beta4] [spkg-check]       File "/sage/pkgs/sagemath-categories/.tox/sagepython-sagewheels-nopypi-norequirements/lib/python3.10/site-packages/sage/doctest/forker.py", line 714, in _run
  [sagemath_categories-10.4.beta4] [spkg-check]         self.compile_and_execute(example, compiler, test.globs)
  [sagemath_categories-10.4.beta4] [spkg-check]       File "/sage/pkgs/sagemath-categories/.tox/sagepython-sagewheels-nopypi-norequirements/lib/python3.10/site-packages/sage/doctest/forker.py", line 1146, in compile_and_execute
  [sagemath_categories-10.4.beta4] [spkg-check]         exec(compiled, globs)
  [sagemath_categories-10.4.beta4] [spkg-check]       File "<doctest sage.categories.additive_monoids.AdditiveMonoids[6]>", line 1, in <module>
  [sagemath_categories-10.4.beta4] [spkg-check]         C.Algebras(QQ).is_subcategory(AlgebrasWithBasis(QQ))
  [sagemath_categories-10.4.beta4] [spkg-check]     NameError: name 'QQ' is not defined
  [sagemath_categories-10.4.beta4] [spkg-check] **********************************************************************

etc...

tornaria commented 2 weeks ago

This change won't take effect for most people because the editable build (via src/setup.py) does not go through sage_build_cython

Why can't src/setup.py use the same code to build cython extensions? In any case, the same line can be added to src/setup.py.

The review question here is: what do you think about enabling all cython warnings as I suggest here? This adds a lot of cython warnings, but at least the Unraisable exception... warning seems a real problem (see https://github.com/sagemath/sage/pull/37792#issuecomment-2053663025 -- after all of those are fixed maybe this warning should be turned into an error).

mkoeppe commented 2 weeks ago

What is going on here:

  [sagemath_categories-10.4.beta4] [spkg-check] **********************************************************************
mkoeppe commented 2 weeks ago

Why can't src/setup.py use the same code to build cython extensions?

Got to ask the author about that.

tornaria commented 2 weeks ago

The review question here is: what do you think about enabling all cython warnings as I suggest here? This adds a lot of cython warnings, but at least the Unraisable exception... warning seems a real problem (see https://github.com/sagemath/sage/pull/37792#issuecomment-2053663025 -- after all of those are fixed maybe this warning should be turned into an error).

dimpase commented 2 weeks ago

"unraiseable" should definitely be turned on.

warnings which are certainly bogus - better not. But OK, all on is good too.

tornaria commented 2 weeks ago

I updated the PR so that all cython warnings are enabled, both in sage_setup and in src/setup.py (for editable builds).

mkoeppe commented 2 weeks ago

Thanks! I'll try it out and take a look.

mkoeppe commented 1 week ago

The output from all the warnings seems somewhat excessive. Perhaps here we should at least try to fix the warnings that come in from the most commonly included .pxd files