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

Remove sage-gdb-commands from src/bin #33627

Closed
mkoeppe opened this issue Apr 2, 2022 · 34 comments
Closed

Remove sage-gdb-commands from src/bin #33627

mkoeppe opened this issue Apr 2, 2022 · 34 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 2, 2022

(from #33625)

Critical because this file is not compatible with editable installs (setup.py develop), see #31049

CC: @tornaria @jhpalmieri @kiwifb @dimpase

Component: scripts

Author: Matthias Koeppe

Branch: d24b928

Reviewer: John Palmieri

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

@mkoeppe mkoeppe added this to the sage-9.6 milestone Apr 2, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Apr 11, 2022
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 22, 2022

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 22, 2022

Commit: a9a2fde

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 22, 2022

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 22, 2022

New commits:

e0c9408Move sage-gdb-commands from src/bin to src/sage/doctest
a9a2fdesrc/sage/doctest/control.py: Get sage-gdb-commands with importlib.resources

@kiwifb
Copy link
Member

kiwifb commented Jul 23, 2022

comment:6

Would sage-gdb-commands not be in a better location in ext_data? It is not a function that is specific to doctests. That's my only reaction so far to that change, but I wouldn't push for the move too strongly if you tell me that it is too hard.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 23, 2022

comment:7

Replying to @kiwifb:

Would sage-gdb-commands not be in a better location in ext_data? It is not a function that is specific to doctests.

The purpose of this file is not documented, and it is only used there

@kiwifb
Copy link
Member

kiwifb commented Jul 23, 2022

comment:8

I just looked at the content of the file for the first time in some years.

r

may be we can just get rid of it if it is the only place it is used. And that's what the gdb manual has to say about what it does https://sourceware.org/gdb/current/onlinedocs/gdb/Starting.html#Starting

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 23, 2022

comment:9

In any case, ext_data is an outdated design and I'd rather not add to it.

@jhpalmieri
Copy link
Member

comment:10

François asks a good question about the purpose of this file. Could we instead just have a variable SAGE_GDB_COMMANDS defined as 'r' in doctest/control.py? Having a file which contains a single character seems a little ridiculous.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 23, 2022

comment:11

The name of the file is passed to gdb, it's a list of commands to execute upon starting. Of course we could also generate this file as a temporary file, but I don't think that's a cleaner design

@kiwifb
Copy link
Member

kiwifb commented Jul 23, 2022

comment:12

Yes, it is all coming back to me now. I guess it is meant to be clever. In any case it is the design we have to deal with and if it is only used from the dosctest folder, then yes it should live there.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 23, 2022

comment:13

Actually the file is also used in src/bin/sage. I missed it because it's written as "${SELF}-gdb-commands"

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

6cc5205src/sage/doctest/control.py: When invoking gdb, use sys.executable

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2022

Changed commit from a9a2fde to 6cc5205

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 23, 2022

comment:15

With recent gdb versions we can just use this option instead and get rid of the command file altogether

  --eval-command=COMMAND, -ex
		     Execute a single GDB command.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2022

Changed commit from 6cc5205 to 9d2c48f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

9d2c48fsrc/bin/sage, src/sage/doctest/control.py: Eliminate use of sage-gdb-commands

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 23, 2022

comment:18

The code handling ./sage --gdb in src/bin/sage has clearly not been tried in a long time. The SAGE_DEBUG logic is broken because this environment variable is only set in the build environment, not in sage -sh

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2022

Changed commit from 9d2c48f to d24b928

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

d24b928src/bin/sage (--gdb): Do not try to run cygdb unless SAGE_DEBUG=yes

@jhpalmieri
Copy link
Member

comment:22

What's the advantage to explicitly including python (a.k.a. sys.executable) in

exec gdb --eval-command="run" --args ...python... sage-runtests --serial --timeout=0 hello_world.py

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 23, 2022

comment:23

Without this change it wasn't working for me at all (on macOS): I got

"/Users/mkoeppe/s/sage/sage-rebasing/src/bin/sage-runtests": not in executable format: file format not recognized

@jhpalmieri
Copy link
Member

comment:24

I can't get it to work on OS X either way. The SPKG doesn't build for me ("C compiler cannot create executables" because "ld: library not found for -ltinfo"). If I run brew install gdb, then I get complaints about things not being codesigned. Anyway, with this branch things fail a bit better than with develop, and the changes make sense to me. Comments from anyone else?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 23, 2022

comment:25

Yes, the gdb SPKG is too old (see #30158). I have also not done the codesigning dance with the homebrew gdb this time to try out whether it really works on macOS

@dimpase
Copy link
Member

dimpase commented Jul 24, 2022

comment:26

isn't gdb pretty much useless with macOS clang, and one should use lldb instead?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 24, 2022

comment:27

Hence #31568

@jhpalmieri
Copy link
Member

comment:28

Let's merge this.

@jhpalmieri
Copy link
Member

Reviewer: John Palmieri

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 25, 2022

comment:29

Thanks!

@vbraun
Copy link
Member

vbraun commented Aug 1, 2022

Changed branch from u/mkoeppe/move_sage_gdb_commands_out_of_src_bin to d24b928

@jhpalmieri
Copy link
Member

comment:31

By the way, we missed an example in developer/doctesting.rst that mentions sage-gdb-commands. See #34295 for a follow-up.

@jhpalmieri
Copy link
Member

Changed commit from d24b928 to none

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

5 participants