Opened 9 years ago

Closed 7 years ago

#8473 closed enhancement (duplicate)

notebook option to upload a .sws file

Reported by: olazo Owned by: iandrus
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: notebook Keywords:
Cc: Merged in:
Authors: Reviewers: Karl-Dieter Crisman, Ivan Andrus
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: Commit:
Dependencies: #13121 Stopgaps:

Description (last modified by jdemeyer)

It would be great for those of us who have installed Sage to be able to double click on these files and have sage open them in the notebook.

Suporting a command on the likes of:

sage -notebook /path/to/worksheet.sws

would be a very good step in this direction.

No patches need to be applied. There is an upstream pull request on github, merged in #13121.

Syntax is

sage -notebook upload="/path/to/worksheet.sws"

The changes to extcode (specifically for the Mac app) have been moved to #11026.

Attachments (3)

trac_8473-sagenb.patch (7.0 KB) - added by iandrus 8 years ago.
trac_8473-extcode.patch (79.8 KB) - added by iandrus 8 years ago.
trac_8473-sagenb.2.patch (7.5 KB) - added by iandrus 8 years ago.

Download all attachments as: .zip

Change History (54)

comment:1 Changed 9 years ago by olazo

  • Description modified (diff)

comment:2 Changed 9 years ago by mpatel

Ticket #693 is related.

comment:3 Changed 8 years ago by kcrisman

What would be needed for this? #693 shows that it's possible to know whether the notebook is running and then open the browser or not based on this. And http://foo.sagenb.org/home/me/upload_worksheet shows that there is a script to take a .sws file and send it straight to the browser. So I say this is definitely doable by someone who knows these scripts. Is this right?

comment:4 Changed 8 years ago by kcrisman

This also led to the following - #9875.

comment:5 Changed 8 years ago by iandrus

  • Authors set to iandrus
  • Status changed from new to needs_review

Depends on #693

I have added support for uploading from a file:// url in order to support sage --notebook upload=FILE.sws. The sagenb patch allows this, and the extcode patch adds support to the Mac application. I had to support file urls so that the upload url could be given as a GET rather than a POST request, and hence be passed to any browser.

The Mac app has a new option to automatically start the server when a sws file is opened. I'm not sure anyone would ever want to turn this off, so I can remove it if it's not useful.

comment:6 follow-up: Changed 8 years ago by iandrus

  • Owner changed from was to iandrus

I should note I only allow file urls when running on localhost because otherwise it doesn't make much sense and could be a security hole.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by kcrisman

Replying to iandrus:

I should note I only allow file urls when running on localhost because otherwise it doesn't make much sense and could be a security hole.

But I can imagine people definitely wanting to upload an sws file to a Sage instance running only in their browser, i.e. from xyz.sagenb.org. And we already allow arbitrary code in a Sage notebook instance! So I don't know whether this would be any less secure than the current situation...

comment:8 Changed 8 years ago by kcrisman

  • Authors changed from iandrus to Ivan Andrus

comment:9 in reply to: ↑ 7 Changed 8 years ago by iandrus

Replying to kcrisman:

Replying to iandrus:

I should note I only allow file urls when running on localhost because otherwise it doesn't make much sense and could be a security hole.

But I can imagine people definitely wanting to upload an sws file to a Sage instance running only in their browser, i.e. from xyz.sagenb.org. And we already allow arbitrary code in a Sage notebook instance! So I don't know whether this would be any less secure than the current situation...

I was probably unclear. With a file url, it would grab the file from the server rather than the local machine, so it's probably not what people want or expect at all. e.g. if you try to upload file:///home/iandrus/sage/sws1.sws, this will only work if the machine the server is running on also has a file at /home/iandrus/sage/sws1.sws, and then only if the file is the same.

It might be nice to add some way to upload a file to a remote server programmatically, but I think that entails logging in and then sending off a POST request from a script, so it has a different feel. I tried writing a simple script using curl a while ago, but I couldn't get it to work for some reason. Perhaps someone knows of a better/different way to accomplish it.

You are right about the security hole though, it's probably the least of our worries :-)

Changed 8 years ago by iandrus

comment:10 Changed 8 years ago by iandrus

Added urlencoding so that files with special characters will work okay.

comment:11 Changed 8 years ago by kcrisman

I'd like to test this, but I'm reluctant to do so until someone who knows something about the notebook takes a look at # 693. Bug days folks?

Changed 8 years ago by iandrus

comment:12 Changed 8 years ago by iandrus

I added zip and txt files to the Mac App as well as support (icons etc.) for Cython files.

comment:13 follow-up: Changed 8 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman

I can taste it now that you reviewed #693. This is great stuff, good work.

Okay, needs work because this option isn't documented. Ordinarily I wouldn't be as strict with NB stuff because so little of it is documented anyway, but I think it is key that notebook? tells us what happens.

I also have a couple questions, but they just indicate my lack of knowledge.

  • I don't understand this change. More precisely, we now have the default startpath = '', but that isn't the same as \. Will that matter somewhere for non-uploading situations?
                            browse_to(old_interface, old_port, old_secure, '/') 
                            browse_to(old_interface, old_port, old_secure, startpath)
    
  • You said you added something to fix special character issues, but I don't know where that is, other than the place where you import urllib - but you only do that in the case that someone needs to log in. Is that the only place it's needed? This could just be my ignorance speaking.
  • Looks like file names with spaces have to be escaped with the syntax `./sage -n /path/to/work\ sheet.sws'
  • I also discovered what might be a bug, or maybe it's a feature... trying to upload a file via ./sage -n /path/to/sws.sws asks for a new password for admin. That is really annoying, esp. if this is only supposed to work on a local machine anyway!

I'd be inclined to separate the new notebook functionality from adding clickable sws files to the Sage App, for review purposes but also because this would have to be part of a new SageNB package, and it would be nice to separate those things out. The logistics are already complicated enough for things.

comment:14 Changed 8 years ago by kcrisman

  • Status changed from needs_review to needs_work
  • Work issues set to document option, only require login when it should be

This doesn't even work - it asks for a password:

notebook(r'''/Users/.../MAT338Day1-2011.sws''',require_login=False)

while notebook() continues to work fine. I would view this as a bug in the case of this syntax.

comment:15 in reply to: ↑ 13 ; follow-up: Changed 8 years ago by iandrus

Replying to kcrisman:

I can taste it now that you reviewed #693. This is great stuff, good work.

:-)

Okay, needs work because this option isn't documented. Ordinarily I wouldn't be as strict with NB stuff because so little of it is documented anyway, but I think it is key that notebook? tells us what happens.

Good point. I have added documentation for notebook?. Does it need documentation in sage --advanced as well?

I also have a couple questions, but they just indicate my lack of knowledge.

  • I don't understand this change. More precisely, we now have the default startpath = '', but that isn't the same as \. Will that matter somewhere for non-uploading situations?
                            browse_to(old_interface, old_port, old_secure, '/') 
                            browse_to(old_interface, old_port, old_secure, startpath)
    

I don't think that should matter, but I changed it to '/' since it makes more sense. In an earlier version of the patch I think I appended to startpath and that's why it was empty.

  • You said you added something to fix special character issues, but I don't know where that is, other than the place where you import urllib - but you only do that in the case that someone needs to log in. Is that the only place it's needed? This could just be my ignorance speaking.

You are right, there was a bug there. I need it both places. Thanks.

  • Looks like file names with spaces have to be escaped with the syntax `./sage -n /path/to/work\ sheet.sws'

Yes. They have to be escaped for the shell so that they appear as one argument. That's the same as any argument to sage -n. However, I should point out that the syntax is sage -n upload="/path/to work sheet.sws". I guess that's why we need documentation, eh? :-)

  • I also discovered what might be a bug, or maybe it's a feature... trying to upload a file via ./sage -n /path/to/sws.sws asks for a new password for admin. That is really annoying, esp. if this is only supposed to work on a local machine anyway!

I think what's happening here is it's starting a new notebook server with /path/to/sws.sws as it's directory or something. I think that's why it's asking for a password.

I'd be inclined to separate the new notebook functionality from adding clickable sws files to the Sage App, for review purposes but also because this would have to be part of a new SageNB package, and it would be nice to separate those things out. The logistics are already complicated enough for things.

That's probably a good idea. I'll split the extcode off into another ticket.

comment:16 Changed 8 years ago by iandrus

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:17 in reply to: ↑ 15 ; follow-up: Changed 8 years ago by kcrisman

Okay, needs work because this option isn't documented. Ordinarily I wouldn't be as strict with NB stuff because so little of it is documented anyway, but I think it is key that notebook? tells us what happens.

Good point. I have added documentation for notebook?. Does it need documentation in sage --advanced as well?

No, because that says the options are as in notebook() in the command line, which covers it.

  • You said you added something to fix special character issues, but I don't know where that is, other than the place where you import urllib - but you only do that in the case that someone needs to log in. Is that the only place it's needed? This could just be my ignorance speaking.

You are right, there was a bug there. I need it both places. Thanks.

Good.

  • Looks like file names with spaces have to be escaped with the syntax `./sage -n /path/to/work\ sheet.sws'

Yes. They have to be escaped for the shell so that they appear as one argument. That's the same as any argument to sage -n. However, I should point out that the syntax is sage -n upload="/path/to work sheet.sws". I guess that's why we need documentation, eh? :-)

Hah! But it's nice that this also works.

  • I also discovered what might be a bug, or maybe it's a feature... trying to upload a file via ./sage -n /path/to/sws.sws asks for a new password for admin. That is really annoying, esp. if this is only supposed to work on a local machine anyway!

I think what's happening here is it's starting a new notebook server with /path/to/sws.sws as it's directory or something. I think that's why it's asking for a password.

Hmm. So if I use the upload= syntax I should be okay?

comment:18 in reply to: ↑ 17 ; follow-up: Changed 8 years ago by kcrisman

  • Status changed from needs_review to needs_work
  • I also discovered what might be a bug, or maybe it's a feature... trying to upload a file via ./sage -n /path/to/sws.sws asks for a new password for admin. That is really annoying, esp. if this is only supposed to work on a local machine anyway!

I think what's happening here is it's starting a new notebook server with /path/to/sws.sws as it's directory or something. I think that's why it's asking for a password.

Yeah, you're right - I found a random new sagenb folder there!

One thing that happens is that your thing checks whether ANY other Sage server is running with the same folder of worksheets. But I had a different Sage version already running through my usual !sage_notebook.sagenb, and that caused an error since it didn't have the syntax, I guess. I don't know that that's a bug.

I think that the extra / is causing problems, though. Here are two things. The second one works.

Downloads/sage-4.7.alpha1/sage -n upload="/Users/.../MAT338Day1-2011.sws"
Downloads/sage-4.7.alpha1/sage -n upload="Users/.../MAT338Day1-2011.sws"

I think that one too many /s is messing things up. But the first one IS the full path to the file, so this could be very confusing. After all,

Downloads/sage-4.7.alpha1/sage -n upload="~/Downloads/MAT338Day1-2011.sws"

gives

exceptions.IOError: [Errno 2] No such file or directory: '/~/Downloads/MAT338Day1-2011.sws'

So we can't use that syntax for the local directory either. Clearly the extra / isn't helping.

comment:19 in reply to: ↑ 18 Changed 8 years ago by iandrus

Replying to kcrisman:

I think that the extra / is causing problems, though. Here are two things. The second one works.

Downloads/sage-4.7.alpha1/sage -n upload="/Users/.../MAT338Day1-2011.sws"
Downloads/sage-4.7.alpha1/sage -n upload="Users/.../MAT338Day1-2011.sws"

I think that one too many /s is messing things up. But the first one IS the full path to the file, so this could be very confusing. After all,

Downloads/sage-4.7.alpha1/sage -n upload="~/Downloads/MAT338Day1-2011.sws"

gives

exceptions.IOError: [Errno 2] No such file or directory: '/~/Downloads/MAT338Day1-2011.sws'

So we can't use that syntax for the local directory either. Clearly the extra / isn't helping.

Hmm. The extra / doesn't cause a problem on my machine, but I have noticed differences in URL handling on earlier version of OS X, so I'll go ahead a remove it.

Changed 8 years ago by iandrus

comment:20 Changed 8 years ago by kcrisman

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Summary changed from Make .sws files clickable to notebook option to upload a .sws file
  • Work issues document option, only require login when it should be deleted

comment:21 Changed 8 years ago by kcrisman

  • Description modified (diff)

comment:22 follow-up: Changed 8 years ago by kcrisman

Ivan, is it very hard to catch a certain kind of error? I get

exceptions.IOError: [Errno 2] No such file or directory: 'Users/.../MAT338Day1-2011.sws'

if I try to upload it with

sage -n upload="Users/.../MAT338Day1-2011.sws"

If I try to upload with

sage -n upload="/Users/.../MAT338Day1-2011.sws"

I get

The resource /upload_worksheet?url=file:///Users/.../MAT338Day1-2011.sws cannot be found.

The same if I do

sage: notebook(upload="...")

So here is what works for me.

  • old patch, not including first slash.

Here is what doesn't work.

  • old patch with a slash.
  • new patch with or without slash.

I'm confused. I really wanted to get this all reviewed but have no idea what is going on, of course.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 8 years ago by iandrus

Replying to kcrisman:

Ivan, is it very hard to catch a certain kind of error? I get

exceptions.IOError: [Errno 2] No such file or directory: 'Users/.../MAT338Day1-2011.sws'

if I try to upload it with

sage -n upload="Users/.../MAT338Day1-2011.sws"

That's what I would expect. Such a thing shouldn't work--that it did before is a mistake :-)

If I try to upload with

sage -n upload="/Users/.../MAT338Day1-2011.sws"

I get

The resource /upload_worksheet?url=file:///Users/.../MAT338Day1-2011.sws cannot be found.

I was finally able to reproduce this. I'm seeing this problem when Sage.app is set to be my browser (so it is logged in) and I run sage --notebook upload=... from the command line. In this case SAGE_BROWSER isn't set correctly so it tries to open the upload page in Safari which isn't logged in. If I then login in Safari and upload again it works. Is this the same problem you are seeing?

So here is what works for me.

  • old patch, not including first slash.

Here is what doesn't work.

  • old patch with a slash.
  • new patch with or without slash.

I'm confused. I really wanted to get this all reviewed but have no idea what is going on, of course.

I don't understand why the old patch would work but not the new one. If it's not the same problem as above then maybe you could try sage --notebook upload=localhost/Users/...` That also works for me and it may work better on older versions of OS X.

comment:24 in reply to: ↑ 23 Changed 8 years ago by kcrisman

  • Status changed from needs_review to needs_work

That's what I would expect. Such a thing shouldn't work--that it did before is a mistake :-)

Okay!

The resource /upload_worksheet?url=file:///Users/.../MAT338Day1-2011.sws cannot be found. I was finally able to reproduce this. I'm seeing this problem when Sage.app is set to be my browser (so it is logged in) and I run sage --notebook upload=... from the command line. In this case SAGE_BROWSER isn't set correctly so it tries to open the upload page in Safari which isn't logged in. If I then login in Safari and upload again it works. Is this the same problem you are seeing?

Aha. Well, I don't have Sage.app even on one of the computers any more (though the icons remain for sws files! what's up with that?), but what did the trick was starting the notebook, getting the error message, leaving the notebook running, and then uploading with the correct syntax a second time.

How do I even check if !SAGE_BROWSER is set? I can't use the command line of that terminal while the server is running, and a new terminal won't have it defined, presumably.

Anyway, this makes me more hopeful this is doable; however, there should not be any connection between having once downloaded the app bundle (perhaps immediately trashed, even) and this syntax working.

comment:25 Changed 8 years ago by kcrisman

  • Description modified (diff)

This needs to be (slightly) rebased for #10652. I can't tell immediately whether the extra filetypes allowed there need any other changes to this patch - my sense is probably not? Anyway, that shouldn't be too hard.

Yeah, the problem on the computer where this doesn't work quite right is probably the same as yours - because I also have a nonstandard browser (well, Firefox) as default there. Switching the default browser to Safari removes the problem.

I really want to give this positive review so that I can review #11026, #10556, and #10555, but it seems like it's not great that one would have to use Safari on Mac for this to work properly (and who knows on other systems where I haven't tested this?).

What's particularly odd is that sage-open (in the scripts directory) is getting the correct browser for me, as far as I can tell, in either case (Safari or FF). But the way sage/misc/viewer.py operates is a little mysterious - sage-native-execute shouldn't have to run sage-open on OS X.

Any thoughts? I'm sorry for the delay in reviewing this, I was busy with Cygwin and running the workshop in my Sage time this summer.

comment:26 Changed 8 years ago by kcrisman

See #11026 for another weird type of error with uploading that only seems to manifest itself with the app part.

comment:27 Changed 8 years ago by kcrisman

I'm getting some other weird behavior, which I think is the same as the previous comment, but I can't be sure because it occurred in a different situation and a different computer. I get "The resource /upload_worksheet?url=file:///Users/karl-dietercrisman/Desktop/Test.sws cannot be found." when going to

http://localhost:8000/upload_worksheet?url=file:///Users/karl-dietercrisman/Desktop/Test.sws

This is after a message about FF not opening because there is already one running. It seems to happen when a separate Mac app is open (not necessarily with server running).

comment:28 Changed 8 years ago by kcrisman

Summary:

  • Still 'needs work'.
  • Needs some kind of respecting of different browsers, though not necessarily universal.
  • Should still work if using a Mac app with #11026?

But I'm excited about this.

comment:29 Changed 7 years ago by iandrus

  • Report Upstream changed from N/A to Reported upstream. Little or no feedback.

comment:30 follow-up: Changed 7 years ago by kcrisman

This is a pull request at the sagenb github site.

I figure this goes back to 'needs review' once (if) it's clarified what patches would still be needed here.

comment:31 follow-up: Changed 7 years ago by kcrisman

Also, those patches should probably be merged into one? Anyway, I found at least one error there -

``http://localhost:8000/upload`` or to fetching

but in the new notebook apparently (?) it's port 8080.

comment:32 in reply to: ↑ 31 Changed 7 years ago by iandrus

Replying to kcrisman:

Also, those patches should probably be merged into one?

Yes. I'm not sure if I can do that now that I have pushed to github though.

Anyway, I found at least one error there -

``http://localhost:8000/upload`` or to fetching

but in the new notebook apparently (?) it's port 8080.

Fixed, thanks.

comment:33 in reply to: ↑ 30 Changed 7 years ago by iandrus

  • Description modified (diff)

Replying to kcrisman:

This is a pull request at the sagenb github site.

I figure this goes back to 'needs review' once (if) it's clarified what patches would still be needed here.

We shouldn't need any of these patches here since the extcode patch was moved to #11026.

comment:34 Changed 7 years ago by roed

  • Report Upstream changed from Reported upstream. Little or no feedback. to Reported upstream. No feedback yet.

comment:35 Changed 7 years ago by kcrisman

  • Report Upstream changed from Reported upstream. No feedback yet. to Completely fixed; Fix reported upstream

I've put this on a more accurate upstream report, given the pull request that has to be reviewed.

comment:36 Changed 7 years ago by kcrisman

The version on the pull request works, at any rate, though it doesn't seem to check correctly for when a server is already running.

Last edited 7 years ago by kcrisman (previous) (diff)

comment:37 Changed 7 years ago by kcrisman

See the pull request for more discussion. It seems to be working great! Just need to check out the new code.

comment:38 Changed 7 years ago by kcrisman

  • Milestone changed from sage-5.1 to sage-5.2
  • Priority changed from minor to major

Okay, the upstream looks good with the one exception of this issue when automatic_login=False but there is still an upload to be done.

Leaving as "needs work" and moving milestone to 5.2 since it depends on #11080 in some sense, though only on the upstream end so not putting it in dependencies.

comment:39 Changed 7 years ago by kcrisman

  • Status changed from needs_work to positive_review

Okay, this was resolved within a half hour! Unfortunately, then it took over five minutes to even get back on Trac for me.

I don't know exactly how this works; we don't make an spkg in this case. I'm putting positive review, but it depends on exactly when this gets from upstream to sage - Jeroen, if you feel comfortable doing so, put it on sage-pending. I just don't want it to get lost.

comment:40 Changed 7 years ago by kini

I've merged the pull request :)

comment:41 Changed 7 years ago by kcrisman

  • Report Upstream changed from Completely fixed; Fix reported upstream to Fixed upstream, but not in a stable release.

comment:42 Changed 7 years ago by jdemeyer

  • Authors Ivan Andrus deleted
  • Milestone changed from sage-5.2 to sage-duplicate/invalid/wontfix
  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Ivan Andrus

comment:43 Changed 7 years ago by jdemeyer

Not really sure what to do either, where is the spkg containing this fix going to end up?

comment:44 Changed 7 years ago by kini

It will end up at #13121.

comment:45 Changed 7 years ago by kini

  • Dependencies set to #13121

comment:46 Changed 7 years ago by jhpalmieri

I'm having problems with this. I've posted comments at the pull request.

comment:47 Changed 7 years ago by kcrisman

The main issue there was resolved at this pull request, so this is fine.

comment:48 Changed 7 years ago by kcrisman

Actually, that was only an issue, not a pull request - it's actually solved at this pull request, with discussion at this issue. See here for the original problem encountered.

comment:49 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_info

I'm assuming nothing here needs to be merged?

Last edited 7 years ago by jdemeyer (previous) (diff)

comment:50 Changed 7 years ago by jdemeyer

Sorry, I got confused because #13121 "depends" on this.

comment:51 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Resolution set to duplicate
  • Status changed from needs_info to closed
Note: See TracTickets for help on using tickets.