Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#10556 closed enhancement (fixed)

Add button to Mac App to add sage executable to PATH

Reported by: iandrus Owned by: iandrus
Priority: major Milestone: sage-5.8
Component: user interface Keywords: mac app
Cc: Merged in: sage-5.8.beta1
Authors: Ivan Andrus Reviewers: Karl-Dieter Crisman, John Palmieri, Nicholas Kirchner
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11026 Stopgaps:

Description (last modified by iandrus)

Since the Mac application comes with a sage executable which is not readily accessible, there should be an easy way to add it to PATH.

It now depends on #11026.

Attachments (3)

add-to-path.png (84.4 KB) - added by iandrus 8 years ago.
trac_10556-extcode.patch (67.2 KB) - added by iandrus 7 years ago.
trac_10556-scripts.patch (1020 bytes) - added by iandrus 7 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 8 years ago by iandrus

  • Status changed from new to needs_review

I added a button to Preferences which allows the user to create a symlink in ~/bin or /usr/local/bin and explains briefly how to do that for users of bash. I have attached a screenshot so that feedback can be given without having to install and build it.

Changed 8 years ago by iandrus

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

Does this depend on #10555 and #8473? Just checking.

I may be able to test and check this all still works on 10.4 PPC next week.

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

Replying to kcrisman:

Does this depend on #10555 and #8473? Just checking.

It doesn't depend on #10555, but it does rely on 8473 (because of nib changes), so we should probably say (for the patchbot):

Depends on #8473

comment:4 in reply to: ↑ 3 Changed 8 years ago by iandrus

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

Replying to iandrus:

Replying to kcrisman:

Does this depend on #10555 and #8473? Just checking.

It doesn't depend on #10555, but it does rely on 8473 (because of nib changes), so we should probably say (for the patchbot):

Depends on #8473

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

Exactly why is this 'needs work' and not 'needs review'?

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

Replying to kcrisman:

Exactly why is this 'needs work' and not 'needs review'?

Because I think it should prompt on startup to add to PATH, provided of course that sage isn't already in PATH. And of course there would also be a "Do not ask again" button. I haven't gotten around to this yet, so if you think it's a bad idea then you can go ahead and review this.

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

  • Status changed from needs_work to needs_review

Replying to iandrus:

Replying to kcrisman:

Exactly why is this 'needs work' and not 'needs review'?

Because I think it should prompt on startup to add to PATH, provided of course that sage isn't already in PATH. And of course there would also be a "Do not ask again" button. I haven't gotten around to this yet, so if you think it's a bad idea then you can go ahead and review this.

I think that could be another ticket. Especially since this will confuse people who don't know what a PATH is.

By the way, I use .profile to add things to the path. Just sayin'.

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

Replying to kcrisman:

Replying to iandrus:

Replying to kcrisman:

Exactly why is this 'needs work' and not 'needs review'?

Because I think it should prompt on startup to add to PATH, provided of course that sage isn't already in PATH. And of course there would also be a "Do not ask again" button. I haven't gotten around to this yet, so if you think it's a bad idea then you can go ahead and review this.

I think that could be another ticket. Especially since this will confuse people who don't know what a PATH is.

That's part of why I haven't gotten to it yet. I'm not sure how to clearly ask the question. You're probably right that it's better to split this into a new ticket. I was planning to do it, but it's harder than I thought (not technically, but design-wise).

By the way, I use .profile to add things to the path. Just sayin'.

Good point. Yet another thing to confuse them with :-)

comment:9 Changed 8 years ago by iandrus

  • Authors set to Ivan Andrus

comment:10 Changed 8 years ago by kcrisman

  • Dependencies set to #8473, #11026

comment:11 Changed 7 years ago by kcrisman

#8473 is solved, see the sagenb upstream pull request.

Putting this to "needs info" because I don't know whether this patch will be compatible with the new notebook.

comment:12 Changed 7 years ago by iandrus

This only depends on #11026, because it changes the interface, which is essentially a binary file. So there should be no problems with compatibility with the notebook.

comment:13 Changed 7 years ago by kcrisman

  • Dependencies changed from #8473, #11026 to #11026
  • Status changed from needs_review to needs_work
  • Work issues set to say to use profile, not bashrc

Status: I like this. But I couldn't get it to work with the ~/bin version. Even after moving my .profile (confirmed in effect) and then creating the .bashrc with only the requested line, it didn't work. Yes, I closed the terminal and opened a new one.

Now, the soft link works, because ./bin/sage works. And the /usr/local/bin version works great. So this is almost ready. In some sense, it IS ready - when I change my .profile to include the requested line, it works!

So I'd just say to change your advice. See this apple.stackexchange question.

By default, Terminal starts the shell via /usr/bin/login, which makes the shell a login shell.
On every platform (not just Mac OS X) bash does not use .bashrc for login shells
(only /etc/profile and the first of .bash_profile, .bash_login, .profile that exists and is readable). 
This is why “put source ~/.bashrc in your .bash_profile” is standard advice. –

Otherwise this is fine, modulo whatever problems in #11026 that would cause this to need rebasing if that were changed.

comment:14 Changed 7 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman

comment:15 follow-up: Changed 7 years ago by jhpalmieri

In sage/ext/mac-app/PreferencePanelController.m, should mkdir ~/bin be replaced by mkdir -p ~/bin?

comment:16 in reply to: ↑ 15 Changed 7 years ago by kcrisman

In sage/ext/mac-app/PreferencePanelController.m, should mkdir ~/bin be replaced by mkdir -p ~/bin?

I can confirm that the directory is in fact created. And what intermediate directories would there be? The permissions seem right too...

drwxr-xr-x   3 k...crisman  staff     102 Jul  5 22:47 bin

and similarly for bin/sage.

So let me be clear that the thing created works; it just doesn't work when you put it in .bashrc unless you go to some trouble, because Mac doesn't seem to respect that very much.

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

comment:17 Changed 7 years ago by jhpalmieri

My suggestion was really in case the directory already existed, you don't want an error, you want to continue with the rest of the command. And I agree with using .profile instead of .bashrc.

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

comment:18 Changed 7 years ago by kcrisman

  • Work issues changed from say to use profile, not bashrc to say to use profile, make sure to have commands work with existing directories

Ok, that seems like a good point. That should be the case for the rm commands as well, though even though there was nothing to remove the commands still worked, but they did raise the message. Maybe rm is different in that regard.

comment:19 Changed 7 years ago by iandrus

  • Status changed from needs_work to needs_review

I have implemented the suggestions which were all good. Since I make all my changes to ~/.bashrc which is included by my ~/.profile, I always forget that ~/.profile is the one that's actually used. Thanks.

comment:20 Changed 7 years ago by kcrisman

  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, John Palmieri
  • Work issues say to use profile, make sure to have commands work with existing directories deleted

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

Okay, this works fine now, in all situations discussed above.

I'm wondering whether you might want to add a comment in the scripts patch about how to REMOVE this from one's path. Just wondering.

Finally, is there a whole bunch of commented code in this? The stuff about spotlight and 10.5 versus 10.6, etc., all inside /* stuff */? Should that be removed, or at least should it be pointed out that this is a comment?

I haven't tested this on anything but Snow Leopard. Probably we should (and it would be easy to do when #11026 is) but I seriously doubt there would be problems, since the main thing is the Unix command.

Changed 7 years ago by iandrus

comment:22 in reply to: ↑ 21 Changed 7 years ago by iandrus

Replying to kcrisman:

Okay, this works fine now, in all situations discussed above.

I'm wondering whether you might want to add a comment in the scripts patch about how to REMOVE this from one's path. Just wondering.

That's a good idea. I've added a patch.

Finally, is there a whole bunch of commented code in this? The stuff about spotlight and 10.5 versus 10.6, etc., all inside /* stuff */? Should that be removed, or at least should it be pointed out that this is a comment?

This was a different approach that I decided not to go with. I thought the code might be useful, but you're right it shouldn't be there.

Changed 7 years ago by iandrus

comment:23 Changed 6 years ago by nkirchner

  • Status changed from needs_review to positive_review

I asked it to create a link in ~/bin. It did.

comment:24 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-feature to sage-pending

comment:25 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-pending to sage-5.8

comment:26 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.8.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:27 Changed 6 years ago by kcrisman

This broke building the Sage app on 10.4 PPC at least.

- (void)alertDidEnd:(NSAlert *)alert returnCode:(NSInteger)returnCode contextInfo:(void *)contextInfo { 

See this sage-devel thread.

comment:28 Changed 6 years ago by kcrisman

  • Reviewers changed from Karl-Dieter Crisman, John Palmieri to Karl-Dieter Crisman, John Palmieri, Nicholas Kirchner

comment:29 Changed 6 years ago by kcrisman

This was followed up on at #14521.

Note: See TracTickets for help on using tickets.