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

REPL shell mode for Windows #23703

Closed
wants to merge 8 commits into from
Closed

REPL shell mode for Windows #23703

wants to merge 8 commits into from

Conversation

GregPlowman
Copy link
Contributor

Replaces stale PR #21294
cc: @stevengj

Summary:

  • JULIA_SHELL can be set to:
    -- cmd
    -- powershell
    -- busybox
    -- "" for no shell, execute command directly (current behavior)

  • cmd is default for Windows users.

  • For Windows, the raw line is processed, rather than converting to Cmd and then re-assembling into a Windows command line. Windows shell commands are passed as a string anyway, so this gives the user more control over any escaping. I think it works quite well.

  • Modified special handling of cd. When there are 2 or more arguments, command line is processed as normal, rather than throwing an error. This allows commands such as cd $dir && $command to be executed.

@ararslan
Copy link
Member

Thanks for revisiting this. In general it's better to update PRs than open new ones but this is fine.

@ararslan ararslan requested a review from stevengj September 13, 2017 23:01
@ararslan ararslan added REPL Julia's REPL (Read Eval Print Loop) system:windows Affects only Windows labels Sep 13, 2017
@stevengj
Copy link
Member

LGTM, but probably someone with more Windows shell expertise like @tkelman should chime in.

@stevengj stevengj added the needs docs Documentation for this change is required label Sep 19, 2017
@stevengj
Copy link
Member

stevengj commented Sep 19, 2017

@tkelman
Copy link
Contributor

tkelman commented Sep 19, 2017

I don't think anything should ever default to cmd.

@stevengj
Copy link
Member

stevengj commented Sep 20, 2017

@tkelman, isn't cmd by far the most familiar interactive command-line environment in Windows?

Wouldn't defaulting to e.g. PowerShell be even more confusing to someone who just wants dir to work (how many Windows users know to type Get-ChildItem)?

Alternatively, I'd be all for bundling something like busybox or toybox on Windows so that people can use the same Unix shell commands on all platforms (with the uniformity outweighing the non-native feel of ls vs. dir). Unfortunately, toybox doesn't support Windows, and people have raised licensing concerns over busybox. (What is the exact problem? The GPL explicitly doesn't extend to "mere aggregation", i.e. shipping two executables together, and the FSF also explicitly says that it doesn't affect one program calling another via fork/exec.)

@tkelman
Copy link
Contributor

tkelman commented Sep 20, 2017

Familiar, maybe, but also the least useful to interact with programmatically.

Powershell has default aliases, both cmd and posix names, for many things. It does have an unfortunate first-time startup latency though.

We already do bundle busybox, but it only gets used by a few tests. I've never been bothered enough by "dir doesn't work in shell mode" to feel the need to rely on it more heavily though. It currently gets left out of a no-GPL build for uniformity's sake, though the viral parts of the GPL don't apply to how we use it.

@stevengj
Copy link
Member

So can we default to busybox, to make the shell mode portable?

@vtjnash
Copy link
Member

vtjnash commented Sep 20, 2017

So can we default to busybox, to make the shell mode portable?

We could also default to Julia's shell mode everywhere, which is also portable.

@stevengj
Copy link
Member

stevengj commented Sep 20, 2017

@vtjnash, but then we get an endless stream of complaints that ls and dir don't work, since getting a directory listing is one of the most common things to do in shell mode.

@vtjnash
Copy link
Member

vtjnash commented Sep 20, 2017

Anything with a binary (such as ls) will work. And as an additional benefit, you could actually use the REPL mode to run actual programs (#10120):

shell> ./julia -e 'print(1)'
bash: syntax error near unexpected token `('

@stevengj
Copy link
Member

stevengj commented Sep 20, 2017

Yes, but the problem is that Windows does not have binaries for ls/dir or other commands that people typically expect in a shell. Hence the complaints and frustration.

One alternative option would be for the shell mode on Windows to preprocess the Cmd object and transform a few common shell functions like ls into calls to busybox.

The basic issue here is to make the default REPL shell mode more usable on Windows.

@GregPlowman
Copy link
Contributor Author

Anything with a binary (such as ls) will work. And as an additional benefit, you could actually use the REPL mode to run actual programs (#10120):

Should also be able to run any binary with this PR.
Here using cmd as shell:

cmd> $JULIA_HOME/julia -e print(1)
1

One alternative option would be for the shell mode on Windows to preprocess the Cmd object and transform a few common shell functions like ls into calls to busybox.

This is effectively what windows cmd does. It "pre-processes" internal commands like dir and calls other programs directly.

It seems some people might want the familiarity of cmd , others want a more flexible and powerful shell (e.g. powershell), and yet others prefer portability (unix-like shell). Satisfying all of these with a single default seems unlikely.

Current PR allows users to select shell mode by setting JULIA_SHELL.
Could change default to "no shell", which is the current behaviour. This would be the least disruptive and would allow users to play around with other shell options, and see if a more popular default emerges over time.

@stevengj
Copy link
Member

stevengj commented Sep 21, 2017

Making the default "no shell" means that the default will seem broken for many users. Furthermore, it is precisely the new users confused by this who will also have a hard time learning about an "unbreak me" JULIA_SHELL option.

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Sep 21, 2017

100% agree with @stevengj. As a purely interactive feature user-friendliness trumps other concerns.

@GregPlowman
Copy link
Contributor Author

Making the default "no shell" means that the default will seem broken for many users.

Precisely. It will seem broken for those users that want cmd, powershell or unix-like behaviour.
Setting some other default will seem broken for those who want something other than that default.

I think cmd might be the better choice for a default. It's familiar, supports internal commands (e.g. dir), and supports running external programs.

Are there any examples of commands that can be run now, but you think can't be run via cmd?
If so, I'd be willing to try to make them work.

@vtjnash
Copy link
Member

vtjnash commented Sep 21, 2017

I have to 100% disagree. I entirely stopped being able to use shell mode once #4635 merged, and now never use it (and I originally implemented it, iirc).

@StefanKarpinski
Copy link
Member

I entirely stopped being able to use shell mode once #4635 merged, and now never use it (and I originally implemented it, iirc).

Then you are clearly not the target demographic for this feature. You've never explained why this feature became "unusable" to you, so it's impossible to make progress towards something that works for you as well as for the many others who are happy with the way this works currently (except on Windows where it's unusable precisely because it has the behavior you claim is "unusable").

@vtjnash
Copy link
Member

vtjnash commented Sep 21, 2017

You've never explained why this feature became "unusable" to you

I opened an issue for that #10120 and you are self-assigned to solve it. Currently, I don't think it's possible to pass the character (.

base/client.jl Outdated
nothing
end

function repl_cmd(iswindows::Val{true}, line::AbstractString, out)
Copy link
Member

Choose a reason for hiding this comment

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

don't use Val

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'm thinking I will just use a single run-time if statement within a common repl_cmd method.

Just for my own edification, why is using Val not recommended? Is it a performance issue?
I thought iswindows() would be known at compile time?
Are there any cases where using Val is OK?

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Sep 21, 2017

That's a bug – saying that it makes shell mode "unusable" is quite dramatic. Feel free to fix said bug, I'm not territorial.

@vtjnash
Copy link
Member

vtjnash commented Sep 21, 2017

If I thought it was fixable, don't you think I would have submitted a PR instead of ceasing to use it? My conclusion at the time was that its behavior was unpredictable-by-design, so I stopped attempting to use it.

@tkelman
Copy link
Contributor

tkelman commented Sep 21, 2017

Calling it unusable on Windows is a bit of an exaggeration. Other than dir, what cmd builtins has anyone ever complained about wanting to use? People should probably use the Julia readdir anyway.

@PallHaraldsson
Copy link
Contributor

@stevengj What is the exact problem? The GPL explicitly doesn't extend to "mere aggregation"

It's puzzling as "mere aggregation" should apply for busybox. We could do everything right, but someone downstream distributing Julia could get in trouble (unless we distribute source code of busybox with?):

https://torquemag.io/2013/03/busybox/

The interesting thing is the financial transaction. You see, it wasn’t enough, in the context of this issue, that Monsoon get away with just “being compliant” but rather also paying a settlement fee. As Ravicher stated,

"Simply coming into compliance now is not sufficient to settle the matter, because that would mean anyone can violate the license until caught, because the only punishment would be to come into compliance."

Sounds familiar on a lot of different fronts. Monsoon was also required to appoint an open source compliance officer on staff as well as publish the source code for the version of BusyBox that they were distributing.

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Sep 25, 2017

What I quoted on busybox sounds like FUD so I dug deeper, busybox copyright lawsuits are used as a lever against Linux kernel GPL violations (even if copyright owners of it don't want to sue).

My reading of this is that even if "mere aggregation" should hold (EDIT: see below, maybe wasn't like that) for the kernel on one hand and busybox on the other, in practice it doesn't (this doesn't apply to Julia.. but not all may realize that and be afraid of using busybox):

http://mjg59.dreamwidth.org/10437.html

The SFC will grant a new license, but on one condition - not only must you provide the source code to Busybox, you must provide the source code to all other works on the device that require source distribution.

https://lwn.net/Articles/478249/

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Sep 25, 2017

If you still want to use busybox, there's:

https://github.com/rmyorston/busybox-w32 "WIN32 native port of BusyBox."

If you distribute [binary] code meant for Windows, I guess the "Linux kernel lawsuit lever" is out.. or not as likely to lead to a lawsuit.

Just speculating on "mere aggregation" issue above, maybe it didn't apply, as busybox intentionally statically links shell command binaries together (we wouldn't link busybox as a whole with Julia.exe); maybe that wasn't the limit, not sure if a firmware would have one giant binary with the busybox + the Linux kernel statically linked..

@PallHaraldsson
Copy link
Contributor

One question, since most users, at least developers will be on Windows 10, is busybox relevant? Can't you just use bash from the Linux subsystem for the shell mode only? Even keeping Julia (otherwise) a pure Windows build (not an ELF build for the subsystem).

@stevengj
Copy link
Member

stevengj commented Sep 25, 2017

@PallHaraldsson, as I explicitly commented above, I don't think either shipping busybox with Julia ("mere aggregation") or executing it via fork/exec would be a problem. (That is, it wouldn't affect the licensing terms of Julia programs.)

The only question in my mind is whether Windows users would prefer a Windows shell (e.g. cmd), or if the superiority of the POSIX shell tools and the advantage of having the same Julia shell commands cross-platform would win. I favor the latter, but I'm obviously biased since I don't normally use Windows.

@GregPlowman
Copy link
Contributor Author

GregPlowman commented Sep 28, 2017

I've tried to update some manual pages, but it seems like something isn't right, and tests are failing.
I wanted to provide a link to JULIA_SHELL in Environment Variables from Shell mode in Interacting With Julia manual page. Perhaps this is the problem?
Can anyone help with this?

EDIT: OK now fixed.
(I had JULIA_SHELL in backticks, which I learnt is for docstrings only, not for document headers)

base/client.jl Outdated
elseif cmd[1] == "cd" && length(cmd) <= 2
repl_cd(cmd, shell, out)
else
run(ignorestatus(isa(STDIN, TTY) ? `$shell -i -c "$(shell_wrap_true(shell_name, cmd))"` : `$shell -c "$(shell_wrap_true(shell_name, cmd))"`))
Copy link
Member

Choose a reason for hiding this comment

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

This could be written a little more cleanly as

ttyopt = STDIN isa TTY ? "-i" : ""
run(ignorestatus(`$shell $ttyopt -c "$(shell_wrap_true(shell_name, cmd))"`))

since -i is the only thing that differs between the two branches.

Copy link
Contributor Author

@GregPlowman GregPlowman Sep 28, 2017

Choose a reason for hiding this comment

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

Yes of course, will update.
Although probably better to use command syntax rather than strings:

ttyopt = STDIN isa TTY ? `-i` : ``

https://discourse.julialang.org/t/interpolating-an-empty-string-inside-backticks/2428/3

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, good call. I had forgotten about that.

@GregPlowman
Copy link
Contributor Author

docs have been updated
code updated to address comments (and no longer using Val)

@GregPlowman GregPlowman mentioned this pull request Dec 11, 2017
@stevengj
Copy link
Member

Would be nice to see this resurrected in some form…

@stevengj
Copy link
Member

Ping. This didn't make it into 1.0, but it is not breaking (just a REPL feature), so it could go into 1.1.

@musm
Copy link
Contributor

musm commented Aug 31, 2018

bump also looking forward to this. ref https://discourse.julialang.org/t/newbi-first-steps-in-julia-for-

@stevengj
Copy link
Member

stevengj commented Mar 26, 2019

Bump? Looks like someone else has to take over this PR if @GregPlowman is too busy on other things these days…

@vtjnash
Copy link
Member

vtjnash commented Mar 26, 2019

c.f. #30614, which implements the escaping function (print_shell_escaped_windows) that's needed to shell out to cmd


if isempty(cmd.exec)
cmd = cmd_gen(eval(Main, shell_parse(line)[1]))
Copy link
Member

Choose a reason for hiding this comment

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

there shouldn't be any calls to eval

Copy link
Member

Choose a reason for hiding this comment

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

eliminated in the updated PR

@stevengj
Copy link
Member

Closing this in favor of the simplified and rebased #31490. I would have updated this PR, but it didn't look like I had the permissions?

@coveralls
Copy link

Coverage Status

Coverage increased (+5.1%) to 92.5% when pulling 16afe32 on GregPlowman:glp/shell_mode_windows into 55ec9fe on JuliaLang:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs docs Documentation for this change is required REPL Julia's REPL (Read Eval Print Loop) system:windows Affects only Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants