Opened 11 years ago

Closed 11 years ago

#9644 closed defect (fixed)

Add error messages and update documentation for spaces in $SAGE_ROOT

Reported by: mpatel Owned by: jason
Priority: major Milestone: sage-4.6
Component: misc Keywords:
Cc: kcrisman, leif, ddrake, schilly, mvngu Merged in: sage-4.6.alpha2
Authors: John Palmieri, Leif Leonhardy Reviewers: John Palmieri, David Kirkby
Report Upstream: N/A Work issues: Update README.txt, Wiki page(s); put info on download page(s)
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by leif)

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:

  • $SAGE_ROOT/README.txt (This file gets into the download directories, too.)

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

Attachments (3)

trac_9644-first_aid_for_spaces_in_SAGE_ROOT-scripts_repo.patch (2.4 KB) - added by leif 11 years ago.
Gives error message on spaces in $SAGE_ROOT. (Draft, but functional. Slightly more friendly.) Apply to scripts repo.
trac_9644-sage-repo-installation.patch (3.7 KB) - added by jhpalmieri 11 years ago.
trac_9644-scripts-ref.patch (1.1 KB) - added by jhpalmieri 11 years ago.
apply on top of other scripts patch

Download all attachments as: .zip

Change History (43)

comment:1 Changed 11 years ago by mpatel

  • Description modified (diff)

After applying

  • sage-sage

    diff --git a/sage-sage b/sage-sage
    a b usage_advanced() { 
    127127    exit 1
    128128}
    129129
    130 . $SAGE_ROOT/local/bin/sage-env   1>/dev/null 2>/dev/null
     130. "$SAGE_ROOT/local/bin/sage-env"   1>/dev/null 2>/dev/null
    131131
    132132if [ $? -ne 0 ]; then
    133133   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: 

comment:2 Changed 11 years ago by mpatel

  • Cc leif added

comment:3 Changed 11 years ago by leif

<flame>

R T F M ! ;-)

(Milestone Sage 6.0)

</flame>

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

comment:4 Changed 11 years ago by leif

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.)

comment:5 Changed 11 years ago by leif

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...

comment:6 Changed 11 years ago by leif

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.)

comment:7 Changed 11 years ago by leif

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

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

comment:8 Changed 11 years ago by leif

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.)

comment:9 Changed 11 years ago by drkirkby

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

Changed 11 years ago by leif

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

comment:10 follow-up: Changed 11 years ago by leif

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.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 11 years ago by 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*.

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?)

comment:12 in reply to: ↑ 11 Changed 11 years ago by leif

  • Cc ddrake schilly added
  • Status changed from new to needs_review
  • Work issues set to Update README.txt, put info on download page(s)

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?

comment:13 Changed 11 years ago by leif

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

comment:14 Changed 11 years ago by schilly

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.

comment:15 Changed 11 years ago by leif

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'.)

comment:16 Changed 11 years ago by mpatel

  • Summary changed from Make Sage run even when $SAGE_ROOT contains spaces to Add error messages and update documentation for spaces in $SAGE_ROOT

comment:17 Changed 11 years ago by mpatel

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.

comment:18 Changed 11 years ago by leif

  • Description modified (diff)
  • Work issues changed from Update README.txt, put info on download page(s) to Update README.txt, Wiki page, Installation Guide; put info on download page(s)

comment:19 Changed 11 years ago by leif

  • Cc mvngu added

comment:20 follow-up: Changed 11 years ago by leif

  • Milestone set to sage-4.6

comment:21 in reply to: ↑ 20 Changed 11 years ago by mpatel

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.

comment:22 Changed 11 years ago by leif

  • Description modified (diff)
  • Work issues changed 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)

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

(It's libgcrypt that fails IIRC.)

comment:23 Changed 11 years ago by jhpalmieri

This looks good to me; positive review for "trac_9644-first_aid_for_spaces_in_SAGE_ROOT-scripts_repo.patch". Trying to get Sage to work with spaces in the path can go on another ticket. Meanwhile, I'm attaching a patch for the installation guide.

comment:24 Changed 11 years ago by drkirkby

  • Authors set to John Palmieri, Leif Leonhardy
  • Reviewers set to John Palmieri, David Kirkby
  • Status changed from needs_review to positive_review

John's happy with Leif's patch. I'm happy with John's changes to the installation guide. So positive review.

comment:25 Changed 11 years ago by mpatel

Thanks, everyone, for working on this.

comment:26 Changed 11 years ago by leif

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.)

comment:27 Changed 11 years ago by jhpalmieri

How about this?

comment:28 Changed 11 years ago by leif

Thanks, very good.

comment:29 Changed 11 years ago by jhpalmieri

  • Status changed from positive_review to needs_work

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...)

comment:30 Changed 11 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

comment:31 Changed 11 years ago by leif

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.)

comment:32 Changed 11 years ago by drkirkby

  • Status changed from needs_review to positive_review

The changes look fine.

The /bin/sh: source: is quite a common error. We should not solve it by redirecting stderr to /dev/null, but removing the word source and 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/sh but does in bash 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!

comment:33 Changed 11 years ago by leif

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... ;-)

comment:34 Changed 11 years ago by mpatel

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

comment:35 Changed 11 years ago by mpatel

  • Status changed from positive_review to needs_work

Changed 11 years ago by jhpalmieri

Changed 11 years ago by jhpalmieri

apply on top of other scripts patch

comment:36 Changed 11 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

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

comment:37 Changed 11 years ago by mpatel

  • Status changed from needs_review to positive_review

Thanks!

comment:38 Changed 11 years ago by jhpalmieri

  • Work issues changed 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)

comment:39 Changed 11 years ago by leif

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.

comment:40 Changed 11 years ago by mpatel

  • Merged in set to sage-4.6.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.