Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#20210 closed enhancement (fixed)

Move memory functions to cysignals

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-7.1
Component: packages: standard Keywords:
Cc: defeo, malb Merged in:
Authors: Jeroen Demeyer Reviewers: Martin Albrecht
Report Upstream: N/A Work issues:
Branch: 4bb8337 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Move all memory-allocation functions like sage_malloc to cysignals (renaming sage_ -> sig_).

Upstream: https://github.com/sagemath/cysignals/releases/download/1.1.0/cysignals-1.1.0.tar.bz2

Change History (20)

comment:1 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/cysignals_memory

comment:3 follow-up: Changed 4 years ago by jdemeyer

  • Commit set to 4bb8337295ed82c93d1a9c9f7173a4c36f97151d
  • Status changed from new to needs_review

New commits:

03458eaUpgrade cysignals package
dce67fcMove memory functions to cysignals
4bb8337Rename sage_malloc -> sig_malloc and friends

comment:4 in reply to: ↑ 3 Changed 4 years ago by jdemeyer

For easier reviewing, I separated the commits:

03458eaUpgrade cysignals package

This is literally just the upgrade of the package.

dce67fcMove memory functions to cysignals

This contains all changes to the Sage library (mostly changing include paths), except for renaming sage_malloc -> sig_malloc and similar.

4bb8337Rename sage_malloc -> sig_malloc and friends

This was made with a simple sed script, it deals only with renaming sage_malloc -> sig_malloc and similar.

comment:5 follow-up: Changed 4 years ago by malb

Why do we have to include "cysignals/memory.pxi" and cannot cimport? Patch looks good, but I didn't check all lines of 4bb8337.

comment:6 in reply to: ↑ 5 Changed 4 years ago by jdemeyer

Replying to malb:

Why do we have to include "cysignals/memory.pxi" and cannot cimport?

Because cysignals/memory.pxi includes cysignals/signals.pxi so it's really by transitivity.

comment:7 Changed 4 years ago by jdemeyer

I am currently in Orsay discussing with Erik Bray and Nicolas Thiéry to see if we can use cimport instead of include for cysignals/signals.pxi but it doesn't look easy.

comment:8 Changed 4 years ago by malb

  • Reviewers set to Martin Albrecht
  • Status changed from needs_review to positive_review

Okay, looks good then. I didn't run tests, though.

comment:9 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/cysignals_memory to 4bb8337295ed82c93d1a9c9f7173a4c36f97151d
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:10 Changed 4 years ago by aly.deines

  • Commit 4bb8337295ed82c93d1a9c9f7173a4c36f97151d deleted

I've been working on ticket #14304 which now depends on this ticket. Since it's been rebased, it now has doctests failing that aren't touched by the ticket. So I ran all the doctests with just this ticket applied and the following one fails:

sage -t src/sage/tests/cmdline.py  # 3 doctests failed

However, the following are failing for #14304

sage -t src/sage/interacts/debugger.py  # 1 doctest failed
sage -t src/sage/tests/cmdline.py  # 14 doctests failed
sage -t src/sage/tests/french_book/nonlinear_doctest.py  # 1 doctest failed
sage -t src/sage/doctest/forker.py  # 1 doctest failed
sage -t src/sage/doctest/test.py  # 1 doctest failed
sage -t src/sage/repl/attach.py  # 2 doctests failed
sage -t src/sage/repl/interpreter.py  # 22 doctests failed
sage -t src/sage/repl/inputhook.pyx  # 2 doctests failed
sage -t src/sage/repl/ipython_extension.py  # 9 doctests failed
sage -t src/sage/repl/ipython_tests.py  # 4 doctests failed
sage -t src/sage/repl/interface_magic.py  # 3 doctests failed
sage -t src/sage/repl/display/formatter.py  # 10 doctests failed
sage -t src/sage/repl/rich_output/backend_ipython.py  # 3 doctests failed
sage -t src/sage/repl/ipython_kernel/install.py  # 1 doctest failed
sage -t src/sage/repl/ipython_kernel/kernel.py  # 4 doctests failed
sage -t src/sage/quivers/algebra_elements.pxi  # 1 doctest failed
sage -t src/sage/dev/trac_interface.py  # 1 doctest failed
sage -t src/sage/typeset/ascii_art.py  # 2 doctests failed
sage -t src/sage/misc/temporary_file.py  # 1 doctest failed
sage -t src/sage/structure/misc.pyx  # 1 doctest failed
sage -t src/sage/structure/sage_object.pyx  # 4 doctests failed
sage -t src/sage/combinat/shard_order.py  # 1 doctest failed
sage -t src/sage/combinat/root_system/root_lattice_realizations.py  # 1 doctest failed
sage -t src/sage/combinat/rigged_configurations/rigged_configurations.py  # 1 doctest failed
sage -t src/sage/combinat/rigged_configurations/rigged_configuration_element.py  # 1 doctest failed
sage -t src/sage/combinat/rigged_configurations/rc_crystal.py  # 1 doctest failed
sage -t src/sage/combinat/rigged_configurations/rc_infinity.py  # 1 doctest failed
sage -t src/sage/interfaces/sage0.py  # 1 doctest failed

Any idea what's up?

comment:11 Changed 4 years ago by jdemeyer

What are the actual failures?

comment:12 Changed 4 years ago by aly.deines

They are:

aly@aly-laptop:~/Sage/sage$ ./sage -t src/sage/tests/cmdline.py 
Running doctests with ID 2016-03-22-16-00-06-1cfb21fa.
Git branch: ticket/20210
Using --optional=ccache,mpir,python2,sage
Doctesting 1 file.
sage -t --warn-long 18.2 src/sage/tests/cmdline.py
**********************************************************************
File "src/sage/tests/cmdline.py", line 106, in sage.tests.cmdline.test_executable
Failed example:
    out.find(version()) >= 0
Expected:
    True
Got:
    False
**********************************************************************
File "src/sage/tests/cmdline.py", line 114, in sage.tests.cmdline.test_executable
Failed example:
    out.find(version()) >= 0
Expected:
    True
Got:
    False
**********************************************************************
File "src/sage/tests/cmdline.py", line 186, in sage.tests.cmdline.test_executable
Failed example:
    out.find(version()) >= 0
Expected:
    True
Got:
    False
**********************************************************************
1 item had failures:
   3 of 235 in sage.tests.cmdline.test_executable
    [234 tests, 3 failures, 30.61 s]
----------------------------------------------------------------------
sage -t --warn-long 18.2 src/sage/tests/cmdline.py  # 3 doctests failed
----------------------------------------------------------------------
Total time for all tests: 30.7 seconds
    cpu time: 0.3 seconds
    cumulative wall time: 30.6 seconds

comment:13 Changed 4 years ago by jdemeyer

Well, that doesn't look related to this ticket... Are you sure you ran make?

comment:14 Changed 4 years ago by aly.deines

Ugg. I think you're right. I'm running make now and will rerun the doctests.

comment:15 Changed 4 years ago by jdemeyer

Do you remember which commands you did run and why you ran them instead of something involving make? Did somebody tell you to run those commands or did you read it somewhere in the documentation? How could we make it more clear that people really should run make before testing or running Sage?

comment:16 follow-up: Changed 4 years ago by aly.deines

I've been running

./sage -b

for most tickets. I thought I only needed to run make when spkg changed, and just forgot to do so on this ticket out of habit of running the Sage build. Should I more generally run make?

comment:17 in reply to: ↑ 16 Changed 4 years ago by jdemeyer

Replying to aly.deines:

I thought I only needed to run make when spkg changed

That is still true.

However: most of the time this spkg change is implicit when changing branches. When you move from a branch based on one Sage version to a branch based on a different Sage version, chances are high that some spkg has changed between those branches.

Should I more generally run make?

At least when changing branches, yes. Once you did this and you then make changes only to the Sage library, then ./sage -b should be sufficient. But run make when in doubt.

Note that you can also use make build instead of make to skip the documentation build which can take a while.

comment:18 Changed 4 years ago by jdemeyer

An easy way to run all doctests safely is to use make ptest or make ptestlong. That will do the build and run all doctests in one simple command.

comment:19 follow-up: Changed 4 years ago by aly.deines

Awesome! Thanks! I might update some of the developer's guide to make this a bit clearer. :)

comment:20 in reply to: ↑ 19 Changed 4 years ago by jdemeyer

Replying to aly.deines:

Awesome! Thanks! I might update some of the developer's guide to make this a bit clearer. :)

That would be really great! Still, the hard part is making people read it.

Note: See TracTickets for help on using tickets.