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

runsnake() cleanup #17735

Open
sagetrac-tmonteil mannequin opened this issue Feb 5, 2015 · 14 comments
Open

runsnake() cleanup #17735

sagetrac-tmonteil mannequin opened this issue Feb 5, 2015 · 14 comments

Comments

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Feb 5, 2015

This ticket aims at cleaning runsnake() function to:

  • update the installation documentation
  • check that runsnakerun is installed before failing silently
  • behave consistently with the state of the preparser

See also #14414 and #17689.

Depends on #9386
Depends on #14414

CC: @nathanncohen

Component: misc

Branch/Commit: u/tmonteil/runsnake___cleanup @ ecaf052

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

@sagetrac-tmonteil sagetrac-tmonteil mannequin added this to the sage-6.5 milestone Feb 5, 2015
@nbruin
Copy link
Contributor

nbruin commented Feb 5, 2015

comment:2

See also #14414. In fact, I'm inclined to close this ticket as "dupe" and address the issues mentioned here on that ticket.

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Feb 6, 2015

Branch: u/tmonteil/runsnake___cleanup

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Feb 6, 2015

New commits:

f21efcc#17735 preparsing behaves consistently with the state of the preparser.
8b6a711#17735 check that the RunSnakeRun program is available.
3640950#17735 string formatting.
6aead48#17735 update installation documentation.
185fa56#17735 missing dot.
ecaf052#17735 link to the profiling thematic tutorial.

@sagetrac-tmonteil

This comment has been minimized.

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Feb 6, 2015

Commit: ecaf052

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Feb 6, 2015

comment:5

Replying to @nbruin:

See also #14414.

Thanks for the pointer, i did not see it.

In fact, I'm inclined to close this ticket as "dupe" and address the issues mentioned here on that ticket.

This ticket adresses different (and more trivial) issues (doc, preparsing, checks) that do not require to wait for a wxPython spkg to be created. It can be merged soon, and rebasing #14414 on this one when it becomes ready should not be hard. Actually, this ticket was first related to #17689 so that we have a simple installation procedure somewhere.

@nbruin
Copy link
Contributor

nbruin commented Feb 6, 2015

comment:6

Replying to @sagetrac-tmonteil:

This ticket adresses different (and more trivial) issues (doc, preparsing, checks) that do not require to wait for a wxPython spkg to be created. It can be merged soon, and rebasing #14414 on this one when it becomes ready should not be hard. Actually, this ticket was first related to #17689 so that we have a simple installation procedure somewhere.

OK, it's also possible to repurpose #14414 for providing the infrastructure to install runsnake in sage itself.

In that case, you would need to do something along the lines of [#14414 comment:23] because checking if runsnake is available and then invoking it in an inappropriate fashion is not an improvement. I have tested that the following works for me (the code on the #14414 comment needed some adjustments)

    ...
    import os, subprocess
    cProfile.runctx(preparse(command.lstrip().rstrip()), get_main_globals(), locals(), filename=tmpfile)
    if os.path.exists(os.path.join(os.environ['SAGE_LOCAL'],'bin','runsnake')):
        subprocess.call(["runsnake",tmpfile])
    else:
        os.system("""sh -c "
          LD_LIBRARY_PATH=$SAGE_ORIG_LD_LIBRARY_PATH; export LD_LIBRARY_PATH
          if [ `uname` = 'Darwin' ]; then
              DYLD_LIBRARY_PATH=$SAGE_ORIG_DYLD_LIBRARY_PATH
          fi
          unset PYTHONHOME
          unset PYTHONPATH
          runsnake %s
        "    
        """%tmpfile)

I really need all of PYTHONHOME, PYTHONPATH, LD_LIBRARY_PATH reset.

Failure depends on /usr/bin/python and $SAGE_LOCAL/bin/python being binary incompatible, so this is something that needs to be tested on a bunch of platforms.

I can help testing/reviewing if you're ok with that approach (but we need to test on multiple platforms).

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Feb 8, 2015

comment:7

Replying to @nbruin:

OK, it's also possible to repurpose #14414 for providing the infrastructure to install runsnake in sage itself.

Why not.

I really need all of PYTHONHOME, PYTHONPATH, LD_LIBRARY_PATH reset.

I like the sage-native-execute approach of Jeroen since it solves the issue for other similar calls, and i wonder whether we could also take the opportunity to also reset the PATH to $SAGE_ORIG_PATH there.

@nbruin
Copy link
Contributor

nbruin commented Feb 16, 2015

comment:8

Setting #9386 as a dependency, since that now offers a saner sage-native-execute. Branch here needs rebasing, by the way. Otherwise, should be good to go. It's probably easiest to rebase on #14414 right away, since that already has the rather small fix in place to properly run runsnake either installed in SAGE_LOCAL or via sage-native-execute.

@nbruin
Copy link
Contributor

nbruin commented Feb 16, 2015

Dependencies: 17746

@nbruin
Copy link
Contributor

nbruin commented Feb 16, 2015

Changed dependencies from 17746 to 9386

@nbruin
Copy link
Contributor

nbruin commented Feb 16, 2015

Changed dependencies from 9386 to #9386

@nbruin
Copy link
Contributor

nbruin commented Feb 16, 2015

Changed dependencies from #9386 to #9386, #14414

@nbruin
Copy link
Contributor

nbruin commented Feb 16, 2015

comment:12

Looking at the implementation of sage.misc.sage_ostools.have_program, we might want to change:

-           if os.access(os.path.join(p, program), os.X_OK):
+           pth=os.path.join(p, program)
+           if os.path.isfile(pth) and os.access(pth, os.X_OK):

since os.access(..., os.X_OK) might return true for a directory and yet this would not be picked up as an executable by the operating system.

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

2 participants