#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, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
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 6 years ago by
- Description modified (diff)
comment:2 Changed 6 years ago by
- Branch set to u/jdemeyer/cysignals_memory
comment:3 follow-up: ↓ 4 Changed 6 years ago by
- Commit set to 4bb8337295ed82c93d1a9c9f7173a4c36f97151d
- Status changed from new to needs_review
comment:4 in reply to: ↑ 3 Changed 6 years ago by
For easier reviewing, I separated the commits:
03458ea Upgrade cysignals package
This is literally just the upgrade of the package.
dce67fc Move 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.
4bb8337 Rename 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: ↓ 6 Changed 6 years ago by
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 6 years ago by
Replying to malb:
Why do we have to
include "cysignals/memory.pxi"
and cannotcimport
?
Because cysignals/memory.pxi
includes cysignals/signals.pxi
so it's really by transitivity.
comment:7 Changed 6 years ago by
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 6 years ago by
- 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 6 years ago by
- Branch changed from u/jdemeyer/cysignals_memory to 4bb8337295ed82c93d1a9c9f7173a4c36f97151d
- Resolution set to fixed
- Status changed from positive_review to closed
comment:10 Changed 6 years ago by
- 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 6 years ago by
What are the actual failures?
comment:12 Changed 6 years ago by
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 6 years ago by
Well, that doesn't look related to this ticket... Are you sure you ran make
?
comment:14 Changed 6 years ago by
Ugg. I think you're right. I'm running make now and will rerun the doctests.
comment:15 Changed 6 years ago by
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: ↓ 17 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
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: ↓ 20 Changed 6 years ago by
Awesome! Thanks! I might update some of the developer's guide to make this a bit clearer. :)
comment:20 in reply to: ↑ 19 Changed 6 years ago by
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.
New commits:
Upgrade cysignals package
Move memory functions to cysignals
Rename sage_malloc -> sig_malloc and friends