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

Move memory functions to cysignals #20210

Closed
jdemeyer opened this issue Mar 15, 2016 · 23 comments
Closed

Move memory functions to cysignals #20210

jdemeyer opened this issue Mar 15, 2016 · 23 comments

Comments

@jdemeyer
Copy link

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

CC: @defeo @malb

Component: packages: standard

Author: Jeroen Demeyer

Branch: 4bb8337

Reviewer: Martin Albrecht

Issue created by migration from https://trac.sagemath.org/ticket/20210

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

Branch: u/jdemeyer/cysignals_memory

@jdemeyer
Copy link
Author

New commits:

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

@jdemeyer
Copy link
Author

Commit: 4bb8337

@jdemeyer
Copy link
Author

comment:4

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.

@malb
Copy link
Member

malb commented Mar 16, 2016

comment:5

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

@jdemeyer
Copy link
Author

comment:6

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.

@jdemeyer
Copy link
Author

comment:7

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.

@malb
Copy link
Member

malb commented Mar 16, 2016

Reviewer: Martin Albrecht

@malb
Copy link
Member

malb commented Mar 16, 2016

comment:8

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

@vbraun
Copy link
Member

vbraun commented Mar 20, 2016

Changed branch from u/jdemeyer/cysignals_memory to 4bb8337

@adeines
Copy link
Mannequin

adeines mannequin commented Mar 22, 2016

Changed commit from 4bb8337 to none

@adeines
Copy link
Mannequin

adeines mannequin commented Mar 22, 2016

comment:10

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?

@jdemeyer
Copy link
Author

comment:11

What are the actual failures?

@adeines
Copy link
Mannequin

adeines mannequin commented Mar 22, 2016

comment:12

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

@jdemeyer
Copy link
Author

comment:13

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

@adeines
Copy link
Mannequin

adeines mannequin commented Mar 22, 2016

comment:14

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

@jdemeyer
Copy link
Author

comment:15

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?

@adeines
Copy link
Mannequin

adeines mannequin commented Mar 22, 2016

comment:16

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?

@jdemeyer
Copy link
Author

comment:17

Replying to @adeines:

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.

@jdemeyer
Copy link
Author

comment:18

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.

@adeines
Copy link
Mannequin

adeines mannequin commented Mar 22, 2016

comment:19

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

@jdemeyer
Copy link
Author

comment:20

Replying to @adeines:

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants