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

Add error messages and update documentation for spaces in $SAGE_ROOT #9644

Closed
qed777 mannequin opened this issue Jul 30, 2010 · 43 comments
Closed

Add error messages and update documentation for spaces in $SAGE_ROOT #9644

qed777 mannequin opened this issue Jul 30, 2010 · 43 comments
Assignees
Milestone

Comments

@qed777
Copy link
Mannequin

qed777 mannequin commented Jul 30, 2010

Reported by Dan on sage-support:

This is an observation about the pre-compiled binaries for Mac OS.

If the (unpacked) sage disk image directory is saved in a folder that has a
space character in its name, for example:

$HOME/Applications/my folder

Then sage will not load properly when the "sage" executable is clicked. The
terminal session begins, but the application doesn't load successfully.

Changing the parent directory name to "my_folder" resolved this issue.

While using space characters in directory names probably isn't all that common
in UNIX or Linux installations, it is more common in Mac OS installations.
Perhaps the installation instructions could be updated to mention this issue? 

This actually appears to be a more general problem:

$ hostname
sage.math.washington.edu
$ pwd
/mnt/usb1/scratch/mpatel/tmp/my sage
$ ./sage
Error setting environment variables by running /mnt/usb1/scratch/mpatel/tmp/my sage/local/bin/sage-env; possibly contact sage-devel (see http://groups.google.com/group/sage-devel).
cat: /bin/sage-banner: No such file or directory
mkdir: cannot create directory `': No such file or directory
cp: cannot create directory `/ipython': Permission denied
Traceback (most recent call last):
  File "./sage-cleaner", line 25, in <module>
    DOT_SAGE = os.environ['DOT_SAGE']
  File "/mnt/usb1/scratch/mpatel/tmp/my sage/local/lib/python2.6/UserDict.py", line 22, in __getitem__
    raise KeyError(key)
KeyError: 'DOT_SAGE'
**********************************************************************
Welcome to IPython. I will try to create a personal configuration directory
where you can customize many aspects of IPython's functionality in:

/ipython
Initializing from configuration /mnt/usb1/scratch/mpatel/tmp/my sage/local/lib/python2.6/site-packages/IPython/UserConfig
WARNING: 

There was a problem with the installation:
[Errno 13] Permission denied: '/ipython'
Try to correct it or contact the developers if you think it's a bug.
IPython will proceed with builtin defaults.
Please press <RETURN> to start IPython.

Similar tickets: #5261 (OS X app bundles), #8303 (command-line arguments).


Documentation to correct / clarify:

Perhaps also put a note onto the download pages for binaries, e.g. http://boxen.math.washington.edu/sage/linux/64bit/index.html

CC: @kcrisman @nexttime @dandrake @haraldschilly @sagetrac-mvngu

Component: misc

Work Issues: Update README.txt, Wiki page(s); put info on download page(s)

Author: John Palmieri, Leif Leonhardy

Reviewer: John Palmieri, David Kirkby

Merged: sage-4.6.alpha2

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

@qed777 qed777 mannequin added p: major / 3 labels Jul 30, 2010
@qed777 qed777 mannequin assigned jasongrout Jul 30, 2010
@qed777

This comment has been minimized.

@qed777
Copy link
Mannequin Author

qed777 mannequin commented Jul 30, 2010

comment:1

After applying

diff --git a/sage-sage b/sage-sage
--- a/sage-sage
+++ b/sage-sage
@@ -127,7 +127,7 @@ usage_advanced() {
     exit 1
 }
 
-. $SAGE_ROOT/local/bin/sage-env   1>/dev/null 2>/dev/null
+. "$SAGE_ROOT/local/bin/sage-env"   1>/dev/null 2>/dev/null
 
 if [ $? -ne 0 ]; then
    echo "Error setting environment variables by running $SAGE_ROOT/local/bin/sage-env; possibly contact sage-devel (see http://groups.google.com/group/sage-devel)."

I get

$ ./sage
----------------------------------------------------------------------
| Sage Version 4.5.2.rc0, Release Date: 2010-07-28                   |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------
**********************************************************************
*                                                                    *
* Warning: this is a prerelease version, and it may be unstable.     *
*                                                                    *
**********************************************************************
ls: cannot access /mnt/usb1/scratch/mpatel/tmp/my: No such file or directory
ls: cannot access sage/devel/sage: Not a directory
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)

/mnt/usb1/scratch/mpatel/tmp/my sage/local/lib/python2.6/site-packages/IPython/ipmaker.pyc in force_import(modname)
     64         reload(sys.modules[modname])
     65     else:
---> 66         __import__(modname)
     67 
     68 

/mnt/usb1/scratch/mpatel/tmp/my sage/ipy_profile_sage.py in <module>()
     14     from sage.misc.interpreter import attached_files
     15 
---> 16     branch = sage.misc.misc.branch_current_hg_notice(sage.misc.misc.branch_current_hg())
     17     if branch:
     18         print branch

/mnt/usb1/scratch/mpatel/tmp/my sage/local/lib/python2.6/site-packages/sage/misc/misc.pyc in branch_current_hg()
   1877     i = s.rfind('->')
   1878     if i == -1:
-> 1879         raise RuntimeError, "unable to determine branch?!"
   1880     s = s[i+2:]
   1881     i = s.find('-')

RuntimeError: unable to determine branch?!
Error importing ipy_profile_sage - perhaps you should run %upgrade?
WARNING: Loading of ipy_profile_sage failed.

sage: 

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jul 30, 2010

comment:3

**R T F M ! ** ;-)

(Milestone Sage 6.0)

It is well documented that $SAGE_ROOT (and therefore $SAGE_LOCAL must not contain spaces; I only wondered a few times if this is checked/catched anywhere, since almost all scripts (especially spkg-install scripts) blindly rely on this. So changing this (allowing spaces) is a big task, which is IMHO not worth doing now. It is in general a bad idea to put spaces in path names (filenames are a little different/less problematic).

-Leif

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jul 30, 2010

comment:4

Ok, it could perhaps be documented in more prominent places, but it's (still) twice in $SAGE_ROOT/README.txt, the second time a bit to mild:

...
MORE DETAILED INSTRUCTIONS TO BUILD FROM SOURCE
-----------------------------------------------
...
3. Extract the Sage source tarball and cd into a directory with no
   spaces in it. [...]

...

RELOCATION
----------

You *should* be able to move the sage-x.y.z directory anywhere you
want. If you copy the sage script or make a symbolic link to it, you
should modify the script to reflect this (as instructed at the top of
the script). It is best if the path to Sage does not have any spaces in
it.
...

And afair it is (or at least has been at some time) better documented in the Installation Guide.

So I would open two tickets (one could be this with changed title):

  • Make sure $SAGE_ROOT does not contain spaces, e.g. in sage-env (blocker).

  • Improve documentation (CAPSLOCK? Add/move remarks to top of files.)

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jul 30, 2010

comment:5

Ouch, I just realized the scripting "logic" is completely insane:

While sage-env is intended to be sourced (rather than called), and sage-sage does this,

  • sage-sage redirects stdout and stderr while sourcing sage-env to /dev/null, though sage-env eventually outputs (valuable) error messages
  • sage-sage tests its "exit status", though it is sourced, not called
  • sage-env actually exits with 1 on error

So even when an error is detected in sage-env, the user will never see the error message, but Sage will simply "silently" exit.

Argh...

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jul 30, 2010

comment:6

With the attached patch I get:

leif@portland:~/Sage /sage-4.5.2.alpha0-j12$ ./sage
Error: The path to the Sage directory ($SAGE_ROOT) MUST NOT contain spaces.
It is currently "/home/leif/Sage /sage-4.5.2.alpha0-j12".
You must correct this by moving Sage (or renaming some directories) first.
Exiting now...
leif@portland:~/Sage /sage-4.5.2.alpha0-j12$ 

(Note the spaces in the path, I simply renamed a component of the path to my Sage directories.)

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jul 30, 2010

comment:7

The error message could of course be a bit more friendly, for example:

"Please correct this by moving Sage (or renaming some directories) first."

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jul 30, 2010

comment:8

An alternative, perhaps simpler, but "less efficient" test for spaces would be:

    [ `echo "X${SAGE_ROOT}X" | wc -w` -ne 1 ]

(This one catches leading and trailing blanks, too, like the shell function in the patch when called properly. But we usually have leading and trailing slashs/path separators in $SAGE_ROOT anyway.)

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Jul 30, 2010

comment:9

I know there was one bit of code in sage which removed one space, but not multiple ones. i.e.

sed 's/ //' 

rather than

sed 's/ //g'

I changed the one occurrence I spotted, but there might be others lucking.

Of course, the best way to solve this is to not have any hacks like this, and just get Sage to build with spaces in paths. If things are quoted properly, this should be possible.

Perhaps there should be an environment variable that can be used to bypass that hack which removes spaces. Then we could slowly find the packages that have problems and correct them one by one. Once they were all corrected, the hack could be removed and Sage would build on paths with spaces in them.

Dave

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jul 30, 2010

Gives error message on spaces in $SAGE_ROOT. (Draft, but functional. Slightly more friendly.) Apply to scripts repo.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jul 30, 2010

comment:10

Attachment: trac_9644-first_aid_for_spaces_in_SAGE_ROOT-scripts_repo.patch.gz

We could require people who update/provide new spkgs to at least check if the upstream supports spaces in path names (I really doubt all will), and to harden the corresponding Sage scripts in that way. But I expect this to be a long lasting process, rather than a goal for any release in the nearer future.

And I have absolutely no idea which "inner" parts of Sage (i.e. Python code) might break on spaces in file or path names.

Of course users that only download and install binaries should somehow get informed, too, e.g. directly on the download page.

@kcrisman
Copy link
Member

comment:11

Of course users that only download and install binaries should somehow get informed, too, e.g. directly on the download page.

And THAT was the point of the original post - that README.txt should probably also mention this issue for downloads, since he downloaded a binary.

Anyway, I'm glad to see this wasn't a platform-specific issue. Probably this ticket should get closed by fixing the manual and adding the error messages, as it were, and then one could open a sage-wishlist milestone ticket for supporting spaces in the path. (The error message may also want to say that certain Sage components would fail otherwise? Or would that be too long?)

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jul 30, 2010

Work Issues: Update README.txt, put info on download page(s)

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jul 30, 2010

comment:12

Replying to @kcrisman:

Of course users that only download and install binaries should somehow get informed, too, e.g. directly on the download page.

And THAT was the point of the original post - that README.txt should probably also mention this issue for downloads, since he downloaded a binary.

Although the README.txt (and other scattered pieces of information) are rarely changed/current, I really thought this was a well-known issue, since I came across that in the documentation many times. (I think it once was better placed, s.t. nobody could miss it, though people tend to read READMEs and installation guides - if at all - after they run into problems... ;-) )

[...] Probably this ticket should get closed by fixing the manual and adding the error messages, as it were, and then one could open a sage-wishlist milestone ticket for supporting spaces in the path.

Yes, that's what I was thinking of, too.

The error message may also want to say that certain Sage components would fail otherwise? Or would that be too long?

As long as it fits onto a 80x25 screen... Feel free to add a "reviewer patch" or make some suggestion and I'll update the patch. I'm quite sure then this could get into 4.5.2.

Someone volunteering for a revised README?

Harald, ideas for the web page?

@nexttime nexttime mannequin added the s: needs review label Jul 30, 2010
@nexttime
Copy link
Mannequin

nexttime mannequin commented Jul 30, 2010

comment:13

Oh, perhaps the Makefile or spkg/install should get some early check, too.

@haraldschilly
Copy link
Member

comment:14

Well, it's possible to add anything somewhere on the webpage, but I think it doesn't look good, or is professional, to list bugs on a download page (especially because it is never read just like the readme).

I don't know the exact problem, but I'm for adding a check at the "sage" script itself -- ahh, i've just seen that above -- and to the early scripts when starting to build sage.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jul 31, 2010

comment:15

The Installation Guide definitely needs to be updated, too:

[...] Make sure there are no spaces in the directory name under which you build. Running from a directory with spaces in its name is supported but discouraged. Building is not possible, since several of the components do not build if there are spaces in the path. [...]

(This snippet is actually from Steps to Install from Source.)

@qed777 qed777 mannequin changed the title Make Sage run even when $SAGE_ROOT contains spaces Add error messages and update documentation for spaces in $SAGE_ROOT Aug 1, 2010
@qed777
Copy link
Mannequin Author

qed777 mannequin commented Aug 1, 2010

comment:17

An aside: For this special situation:

  1. Build Sage with no spaces in SAGE_ROOT.
  2. Rename SAGE_ROOT to contain space(s).

the patches here should get Sage to start and get sage -b, sage -clone, sage -docbuild, and sage -t(p) commands to work. Except for some SYMPOW and GAP-related tests, the long doctests pass. It's likely that fixing these components requires spkg updates.

I'm sure there are other problems to fix, and these patches certainly won't allow Sage to build with spaces in SAGE_ROOT. But the exercise suggests that to fix most (all?) Python code, we should focus on system calls, e.g., os.{popen*,system}, subprocess.{call,Popen}, etc. Of course, we can open other tickets for these changes.

@nexttime

This comment has been minimized.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 19, 2010

Changed work issues from Update README.txt, put info on download page(s) to Update README.txt, Wiki page, Installation Guide; put info on download page(s)

@nexttime nexttime mannequin added this to the sage-4.6 milestone Aug 20, 2010
@qed777
Copy link
Mannequin Author

qed777 mannequin commented Aug 20, 2010

comment:21

Thanks, Leif. I'd like to focus, for now, on releasing 4.5.3 and then on the PARI upgrade for 4.6.alpha0. I'll try to work on this ticket after 4.5.3 is out.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 20, 2010

Changed work issues from Update README.txt, Wiki page, Installation Guide; put info on download page(s) to Update README.txt, Wiki page(s), Installation Guide; put info on download page(s)

@nexttime

This comment has been minimized.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 20, 2010

comment:22

Btw: If you want to have fun, try compiling Sage with -O2 contained in $SAGE_ROOT.

(It's libgcrypt that fails IIRC.)

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 21, 2010

comment:26

So we are all happy. :)

Minor correction: Swap from and to in "Another approach is to create a symbolic link ...".
(This has been in before. IIRC there are somewhere more instances of that, but never mind.)

@jhpalmieri
Copy link
Member

comment:27

How about this?

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 21, 2010

comment:28

Thanks, very good.

@jhpalmieri
Copy link
Member

comment:29

The invocation of sage-env from the top-level makefile is causing errors. On sage.math:

. local/bin/sage-env && sage-starts && ./sage -tp 0  -sagenb -long devel/sage/doc/common devel/sage/doc/en devel/sage/doc/fr devel/sage/sage 2>&1 | tee -a ptestlong.log
local/bin/sage-env: 70: Syntax error: Bad function name
make: *** [ptestlong] Error 2

On my mac:

. local/bin/sage-env && sage-starts && ./sage -tp 0  -sagenb -long devel/sage/doc/common devel/sage/doc/en devel/sage/doc/fr devel/sage/sage 2>&1 | tee -a ptestlong.log
local/bin/sage-env: line 77: `contains-spaces': not a valid identifier
make: *** [ptestlong] Error 2

On t2.math:

. local/bin/sage-env && sage-starts && ./sage -tp 0  -sagenb -long devel/sage/doc/common devel/sage/doc/en devel/sage/doc/fr devel/sage/sage 2>&1 | tee -a ptestlong.log
/bin/sh: contains-spaces: is not an identifier
make: *** [ptestlong] Error 1

In all cases, changing "contains-spaces" to "contains_spaces" seems to fix the problem. I'm attaching a small patch to do this. (On Solaris systems, I also see a new warning, presumably because we're not redirecting output to /dev/null: after typing "make ptestlong", I see

Build finished.  The built documents can be found in /scratch/palmieri/sage-4.5.3.rc0/devel/sage/doc/output/html/fr/a_tour_of_sage
. local/bin/sage-env && sage-starts && ./sage -tp 0  -sagenb -long devel/sage/doc/common devel/sage/doc/en devel/sage/doc/fr devel/sage/sage 2>&1 | tee -a ptestlong.log
/bin/sh: source: not found
Testing that Sage starts...
Yes, Sage starts.
Testing that Sage starts...
Yes, Sage starts.
Global iterations: 1
File iterations: 1

Note the line /bin/sh: source: not found. Is this important? If so, we should deal with it on another ticket...)

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 22, 2010

comment:31

Thanks, I wanted to make exactly the same change weeks ago... (replacing the dash in the name by an underscore).

Also, source should be replaced by . (period). (At #9960 or here.)

(Note that this error message has always been there; you just had to look into /dev/null... ;-) Redirecting stderr to Nirvana is rarely a good idea.)


Although the Makefile shouldn't break the assumption that we're using bash; so either sage-env should be removed there, or the line should be bash -c ..., or /usr/bin/env bash -c .... There are various ways to fix that.

I wonder if sage-starts shouldn't use bash (instead of sh) and itself source sage-env; then we could drop . local/bin/sage-env && from TESTPRELIMS. (This would IMHO be cleaner.)

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Sep 22, 2010

comment:32

The changes look fine.

The /bin/sh: [](../tree/master/) is quite a common error. We should not solve it by redirecting stderr to /dev/null, but removing the word sourceand replacing it with a dot. I suspect there are many instances of this lucking around. The problem is the command does not exist in/bin/shbut does inbash` See below.

Here my default shell is bash, so source exists as a built in shell command.

kirkby@t2:64 ~$ source
-bash: source: filename argument required
source: usage: source filename [arguments]

Now change my shell to /bin/sh

kirkby@t2:64 ~$ sh
$ source
source: not found
$ 

I'd prefer to use the more portable . (dot) which will achieve the same, but is not a bashism. These should certainly be addressed on another ticket. The fact they are not producing any known errors, makes me wonder how important it is to source the files in the first place!

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 22, 2010

comment:33

Sourcing sage-env in the Makefile is only needed to set up SAGE_ROOT for sage-starts, which calls a Python script (sage-location).

The other Sage scripts source sage-env themselves.

As said before, best would be to change sage-starts (and the Makefile) on another ticket; replacing source by . could be done at #9960 I think.

An even nicer solution would just "source" sage-env from the Python script (by invoking a shell that does this and echoes the necessary variables), also removing . sage-env && from the Makefile.

You can hardly touch a piece of Sage without experiencing other things to fix... ;-)

@qed777
Copy link
Mannequin Author

qed777 mannequin commented Sep 29, 2010

comment:34

Could someone please update the commit strings of the "installation" and "ref" patches to be a bit more descriptive and restore the positive review?

@qed777 qed777 mannequin added s: needs work and removed s: positive review labels Sep 29, 2010
@jhpalmieri
Copy link
Member

Attachment: trac_9644-sage-repo-installation.patch.gz

Attachment: trac_9644-scripts-ref.patch.gz

apply on top of other scripts patch

@jhpalmieri
Copy link
Member

comment:36

Here are new commit strings. Can we restore the positive review now?

@qed777
Copy link
Mannequin Author

qed777 mannequin commented Sep 29, 2010

comment:37

Thanks!

@jhpalmieri
Copy link
Member

Changed work issues from Update README.txt, Wiki page(s), Installation Guide; put info on download page(s) to Update README.txt, Wiki page(s); put info on download page(s)

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 29, 2010

comment:39

I've updated some Wiki pages a while ago.

Harald said we shouldn't "announce bugs" on the download pages; I think a simple comment on where to currently not install Sage shouldn't be interpreted as such.

@qed777
Copy link
Mannequin Author

qed777 mannequin commented Sep 29, 2010

Merged: sage-4.6.alpha2

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

4 participants