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

Tcsh support #67

Merged
merged 4 commits into from
Mar 2, 2021
Merged

Tcsh support #67

merged 4 commits into from
Mar 2, 2021

Conversation

mnlevy1981
Copy link
Contributor

Cecile H reported problems running jupyter forward, and @andersy005 tracked it down to a few bash commands that we were executing on the remote machine (she uses tcsh by default). We were able to get her running with some kludgy updates, but this PR cleans everything up.

Testing: I created a VM where my default shell was tcsh, and verified that I could not use jupyter-forward out of the box. After the following commits I was able to use jupyter-forward to get to my virtual machine, and could still use it to connect to machines where my default shell is bash.

Fixes #66

1. Rather than command -v, calling sh -c "command -v" works in both bash and
   tcsh
2. "2>&1" is bash-specific, but ">&" is recognized by both

I also commented out the check for whether TMPDIR exists; that will also need
to be handled in a shell-agnostic manner.
It doesn't appear to do anything, so we'll let everything run in user's default
shell.
I added an extra step: check to see if TMPDIR is a defined environment variable
(via printenv); if the variable exists, we then check to see if the directory
exists by calling "cd".
@mnlevy1981 mnlevy1981 requested a review from andersy005 March 1, 2021 22:48
@mnlevy1981
Copy link
Contributor Author

Note that I left the --shell command line argument because it is used with --launch-command but it is no longer part of self.run_kwargs

self._jupyter_info(check_jupyter_status)
if self.dir_exists('$TMPDIR'):
if self.envvar_exists('TMPDIR') and self.dir_exists('$TMPDIR'):
self.log_dir = '$TMPDIR'
else:
self.log_dir = '$HOME'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should double check to make sure $HOME exists and is pointing to a directory too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If $HOME does not exist, should we abort? I could do something like

            if self.envvar_exists('TMPDIR') and self.dir_exists('$TMPDIR'):
                self.log_dir = '$TMPDIR'
            elif self.envvar_exists('HOME') and self.dir_exists('$HOME'):
                self.log_dir = '$HOME'
            else:
                console.log(
                    '[bold red]Can not determine directory for log file'
                )
                sys.exit(1)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if both $TMPDIR and $HOME are invalid, we should probably raise an error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a more informative error, we can be more verbose in Can not determine directory for log file' by specifying that both $TMPDIR and $HOME were found to be undefined or pointing to non-existing directories....

Also added better log message in the event that neither $TMPDIR nor $HOME are
available for creating .jupyter_forward/ (with no place for the log files to
go, we expect jupyter-forward to abort)
Copy link
Member

@andersy005 andersy005 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your work on this, @mnlevy1981! This looks great and hopefully addresses other shell specific issues :)

@andersy005 andersy005 merged commit a2847f8 into ncar-xdev:main Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jupyter-forward does not work for tcsh users
2 participants