Skip to content

Conversation

@GregPlowman
Copy link
Contributor

There has been much discussion regarding REPL shell mode for Windows:

In my reading, I think there seems to be 2 main issues:

  • cmd is difficult to work with, especially when escaping special characters
  • powershell is better but is quite slow to startup

This proposal is to enable users to opt-in (via JULIA_SHELL) to using cmd or powershell as the command interpreter.

The default shell mode for Windows users is unchanged.
If JULIA_SHELL environment variable is not explicity set, then shell mode executes commands directly, as is the case now.

There are probably many cases where shell escaping does not work properly, but simple commands including the built-ins should work. For anything more complex it would probably be easier to launch the shell (via ;cmd or ;powershell) and run commands directly from there.

I haven't done extensive testing but the following commands and paths work:

Command cmd powershell
ls
dir
dir c:\\windows
dir "c:\program files"
dir c:\\program\ files
dir "c:\Windows" "c:\program files"

If there is no real appetite for progressing with this proposal, please feel free to close.
I just think that minimal opt-in support for simple commands on Windows would be better than what is available now.

Unfortunately I cannot build Julia and test this directly.
My tests have been on similar changes to v0.5.0.

RFC/WIP: Opt-in to shell for REPL shell mode on Windows
@ararslan ararslan added REPL Julia's REPL (Read Eval Print Loop) system:windows Affects only Windows labels Feb 24, 2017
@ararslan ararslan requested a review from tkelman February 24, 2017 06:13
@stevengj
Copy link
Member

Don't we also have busybox (#7446), which would offer the possibility of using Unix shell programs? This way the basic shell functions would be somewhat platform-independent.

@StefanKarpinski
Copy link
Member

That has been suggested (iirc) but Windows folks seem to want their shell prompt to work like cmd.

@stevengj
Copy link
Member

It seems like 99% of what people want is ls and cat (or dir and type). It seems a little sad on Windows to have to "opt in" to an "unbreak me" environment variable just to get these.

@StefanKarpinski
Copy link
Member

I'm totally fine with it, but then we get complaints that dir doesn't work.

@tkelman
Copy link
Contributor

tkelman commented Feb 24, 2017

cmd is needlessly complicated to implement correct quoting and escaping for, the last time this came up

busybox is GPL so not ideal, and I wouldn't really call a posix compatibility shim for a non-posix os "platform-independent"

@stevengj
Copy link
Member

busybox is GPL so not ideal

I guess there is toybox, though I don't know if anyone has ported it to mingw? But if we are already including busybox...?

I wouldn't really call a posix compatibility shim for a non-posix os "platform-independent"

I don't understand this comment. Platform-independent behaviors are always implemented on top of platform-specific shims. If you want a platform-independent shell mode, this is how you would do it, no?

(Or we could implement our own shell that is incompatible with everything, of course.)

@GregPlowman
Copy link
Contributor Author

It seems a little sad on Windows to have to "opt in" to an "unbreak me" environment variable just to get these.

Yes, I agree.

I guess the purpose of this proposal was to give Windows users the ability to select their shell (just as Linux users can). I have changed the title accordingly.

Under this proposal, users could choose: no shell, cmd or powershell (maybe busybox could be added)

At the moment none of these are ideal and each has it's shortcomings, but maybe that's a separate issue.

My concern is that an ideal single solution might be far off. In the meantime Windows users might be able to experiment, and maybe even submit PRs to improve one or more options.

On the other hand, I do understand proceeding with non-ideal changes is not quite right.

@GregPlowman GregPlowman changed the title RFC/WIP: Opt-in to shell for REPL shell mode on Windows RFC/WIP: Allow Windows users to specify JULIA_SHELL for REPL shell mode Feb 25, 2017
@GregPlowman
Copy link
Contributor Author

GregPlowman commented Mar 6, 2017

busybox is great!

This way the basic shell functions would be somewhat platform-independent.

yes, I like this.

I'm totally fine with it, but then we get complaints that dir doesn't work.

I can't speak for other Windows users, but I'd be perfectly happy with unix-style commands. I'm learning Julia, so I think I can learn a few commands like ls and cat.
Also at the moment, Windows users can't use any built-in shell commands.

I've updated this PR to add support for busybox via setting JULIA_SHELL.

It seems a little sad on Windows to have to "opt in" to an "unbreak me" environment variable just to get these.

Well it's a simple change to make busybox the default.

@tkelman
Copy link
Contributor

tkelman commented Mar 6, 2017

the version of cmd /C here isn't going to handle arguments correctly, see the complexity of the past versions of this

base/client.jl Outdated
elseif shell_name == "powershell"
run(ignorestatus(`$shell -Command $(shell_escape(cmd))`))
elseif shell_name == "busybox"
run(ignorestatus(`$shell $cmd`))
Copy link
Member

@stevengj stevengj Mar 8, 2017

Choose a reason for hiding this comment

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

Do we need to do any special escaping for busybox on Windows?

If Julia bundles busybox on Windows, can we default to that (with the correct path if it is buried somewhere in the julia directory)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to do any special escaping for busybox on Windows?

Yes, for busybox now using full sh applet with shell_escape()

If Julia bundles busybox on Windows, can we default to that (with the correct path if it is buried somewhere in the julia directory)?

Default is now set to busybox using full path to julia bin sub-directory.
(Full path might not be necessary since bin sub-directory is added to PATH)

@GregPlowman
Copy link
Contributor Author

Summary of updates

  • default is busybox using full path to Julia bin sub-directory
  • for busybox, now using sh applet with shell_escape
  • cmd is disallowed: issue warning, then reset to default busybox

Would some users still want old behavior (bypass shell and directly execute commands)?
If so can set ENV["JULIA_SHELL"] = ""

shell = shell_split(get(ENV,"JULIA_SHELL",get(ENV,"SHELL","/bin/sh")))
shell_name = Base.basename(shell[1])
if is_windows()
default_shell = string("\"", joinpath(JULIA_HOME, "busybox"), "\"")
Copy link
Contributor

Choose a reason for hiding this comment

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

spaces, not tabs

@tkelman
Copy link
Contributor

tkelman commented Mar 12, 2017

Shouldn't default to busybox in a no-gpl build where we don't include it. I'm also not so sure Windows users want to default to unix shell behavior either.

@GregPlowman
Copy link
Contributor Author

Shouldn't default to busybox in a no-gpl build where we don't include it.

Oh I see.
Can easily update to check whether busybox is installed before setting default.

I'm also not so sure Windows users want to default to unix shell behavior either.

I see your point. And also default for no-gpl build would be different from normal build.
OTOH, I'm not sure Windows users like the current "no-shell" default.

Perhaps it's better to revert to original opt-in approach? i.e. Keep the current "no-shell" default.

* fix regressions due to replacing too many slots with SSAValues

* Work around bug in tar 1.29 with --no-recursion and -T (#20942)

causing duplicate files and including directories it shouldn't be

* Fix printing of indices in the depwarn for to_indexes (#20960)

* Use proper alignment when copying data to a variable.

Using align=1 is very expensive on hardware without unaligned ld/st (eg. GPUs).

* Lower `x^n` (literal n) to `Base.literal_pow(^, x, Val{n})` (#20889)

Fixes #20882

* Add section on type piracy to style guide (#20967)

* Add section on type piracy.

* Add examples of type piracy.

* Resolve ambiguity between * methods associated with RowVectors.

* use explicit checks instead of try/catch to detect BLAS vendor

* Nix ambiguity between \ methods.

* fix broken link to xterm 256 colors in NEWS

* Resolve ambiguities between checkbounds_indices methods.

* Resolve ambiguity between tuple constructors.

* Fix handling of `allow_bottom_tparams` in `isambiguous`

The former version failed to detect that certain type parameters were Bottom (for example, in the case in the added docstring for `isambiguous`).

* Resolve ambiguity between colon methods.

* Fix broadcast with RowVectors of matrices (#20980)

Fix #20979. Amusingly, this bug is a direct result of `transpose` being recursive.

* pkg: factor url_or_pkg logic into its own function and test it

* `abstract` -> `abstract type` (#21002)

* Upgrade Documenter to 0.9.1 (#20997)

fixes some windows path issues

* Fix `typeintersect` involving `TypeVar`s bound to non-types

Modify `var_gt` and `intersect_var` to cope with cases where a `TypeVar`
has been deduced to be a non-type constant as is compared or intersected
with another `TypeVar` or vice versa.

* fix #20998

This was a case where an unknown typevar was incorrectly inferred to have the
value `Any`.

* fix #20992, compiler hang

This was due to type intersection returning a type containing a free
variable.

* Make broadcast over sparse and rowvecs sparse

* fix #20999, allow type redefinition with UnionAll-typed fields

* Clarify data sharing behavior of vec (#21030)

[ci skip]

* NEWS.md: various fixes (#20996)

* fix #21028, static parameter value incorrectly set to typevar

This issue was caused by 6fb2c67.

* Improve printing of manually thrown `MethodError`s

Make sure the world age is only mentioned for valid exception world age and when
exception world has no matching method.

* Update NEWS.md w.r.t. removal of localize_vars [ci skip] (#21034)

Changes related to PR #19594

* Correct error message when adding/removing processes from workers (#21033)

* Clarify some license exceptions

FFTW wrappers were moved and mostly rewritten - the Julia wrappers
are probably not GPL? ref 481f351
and #8248 and #12201

make note of grisu derived license in top-level LICENSE.md
and update links, repo was moved from github.com/floitsch to github.com/google

umfpack and cholmod code were moved from base/linalg to base/sparse

* RFC: Added recursive transpose to `Diagonal` (#21019)

* Added recursive transpose to `Diagonal`

`Diagonal` might be used to represent a block-diagonal matrix structure.

* Fixes plus expm, logm, sqrtm

* fix strtod indentation and mention it in LICENSE.md

ref #5988

* Change default proxy settings to auto (#21044)

* documentation: change `immutable` to `struct` (#21056)

* Fix errors in, and run, add_license_to_files.jl

* Fix line numbers and schurfact `show` output in doctests

* Fix links in documentation

smaller version of 0cc7d79

use --color=yes when running linkcheck

* Extensible and fast date parsing (#20952)

* Refactor date parsing to be fast and extensible

* Deprecate parse(::AbstractString, ::DateFormat)

* fixup

* Switch to datatype_name

* Move towards consistent terminology

* Rename parse(::Vector, ...) to parse_components

* Internal parse funcs now take and return position

* Documentation for internal functions

* Corrections to documentation

* Corrections from review

* More details about raise

* Remove outdated comment

* Cleanup function signatures using DateLocale (#21011)

* fix `return_type_tfunc` in the case where no methods match

also fix `return_type` on Builtins; this was another disparity with
  `return_type_tfunc`

* Introduce temporary if necessary to inline `_apply`

* use `newvar!` instead of `add_slot!`

* add test

* Download failure with curl no longer create a file

* fix #21054, parse/lowering problem with `<:T` syntax

* use newest world callback on_done (#21027)

* use newest world callback on_done

* use QuoteNode to prevent parameters being expanded

* also quote on_done

* fix #20611, type intersection inferring wrong parameter value

* fix #21081, quadratic slowdown in `remove_redundant_temp_vars!`

* ensure the gc-rooting pass does not discard derived roots

fix #21015

* Carry openblas power assembly fixes

from OpenMathLib/OpenBLAS#1098

* Revert "Add patch to set `-O2` for suitesparse build (#20165)"

This reverts commit 145eae8.

This was actually a bug in the power kernels in OpenBLAS,
not a GCC or SuiteSparse problem

* Respect format width for milliseconds (#21070)

Fixes #21001

* wrap long lines in base/linalg/lu.jl

line breaks in definitions of stridedarray aliases

and a couple lines in test/parse.jl

* wrap long lines in base/abstractarray.jl

* 4 space indent in go_benchmark.jl

* put comments at the same indentation level as code in base/linalg/givens.jl

* empty line before jldoctest openers in base/reflection.jl

* Throw an exception for invalid hostnames in addprocs. (#21021)

* throw an exception for invalid hostnames in addprocs.

* clean up some one-liners in string functions

remove unnecessary spaces inside [] in contains doctest

fix nonstandard indentation in utf8proc tests

top level testset in a file under test/ should be redundant now

fix some typos and grammar in a few comments

add some spaces in arnoldi tests

combine sentences instead of starting one with And

change bitstype to primitive bits type in simd-types docs

fix plural subtypes

fix plural or other types

fix retrhow and unsatifiable typos in base/pkg/resolve.jl

code highlighting in types manual section

* rename oid to oid_ptr in fetchhead_foreach_callback

add a few spaces after # comment openers

spell out acronyms in compiler devdocs

make spacing more conventional in test/replutil.jl

* use === for comparison to nothing

* Use `typejoin` of the field types for `eltype` of heterogeneous `Tuple`s

* Move `rewrap_unionall` in `eltype(::Type{<:Tuple})`

* Improve task switching times (#21111)

* Only create file upon wget success

* Fix typo in inference

* Improve performance of skipchars for small inputs

This mostly solves the problem, now the function is only significantly (2x) slower when there are no characters to skip, otherwise it's mostly the same for 1 or 2 characters and faster for 3 or more.

Also improve the tests a little.

* fix #21088, missing `widenconst` in `return_type` (#21115)

* Handle `Union` of `Tuple`s in `eltype(::Type{<:Tuple})`

* fix #21074, constant fold more expressions

* also for #21074, improve `getfield_elim_pass!` to handle more kinds of values

* fix a bug in the t-func for `nfields`

* fix a bug in `getfield` t-func on invalid calls

* improve constant folding of SimpleVector length and `nfields`

* fix some issues around inlining constants

avoid inlining large constants

* fix unary `<:` and `>:` call syntax

* fix FFTW crash after set_num_threads

* cosmetic: add a couple of newlines at file ends

* rename allow_bottom(_tparams)? => ambiguous_bottom

* isambiguous: make ambiguous_bottom a keyword argument

This matches detect_ambiguities – yes, isambiguous is unexported,
but we know people will still use it and we may as well have
consistent signatures here.

* deprecate old arguments to isambiguous, detect_ambiguities

* test ambiguous_bottom options to isambiguous, detect_ambiguities

* swap default for ambiguous_bottom from true to false

* fix #21118, subtyping issue with diagonal variable with abstract lower bound

In `Tuple{Vararg{T}} where T`, `T` is diagonal. In the case in this issue,
type intersection was giving such a variable an abstract lower bound. That
was wrong, but it also created a type that could not be subtyped properly
since the given bound on the variable contradicted the requirement that it
be concrete. So for now at least, given e.g. `Tuple{T,T} where T>:Real`,
simply drop the diagonal-concrete rule.

* OSX: Add both RPATH entries to `julia`, don't overwrite

See
30ba746#commitcomment-21430419
for more details

* Add tests for download

* fix #21132, intersection hang due to x == Pair{x} constraint

* Fix stride for vectors in BLAS.ger! and BLAS.trsv! (#21125)

Fixes #21122

* add devdocs on "diagonal" types

[ci skip]

* test/socket: wait_with_timeout to provide more debugging info

* Immediately expand JULIAHOME in Makefile (#21140)

* fix whitelist of expression heads in `lambda-optimize-vars!`

`decl` and `ssavalue` were omitted by mistake.
This helps #15276.

* compress AST in memory, not just on disk

sysimg size will be exactly the same,
but seems to be pretty significant savings in memory and initial GC time
(about 10% on each)

* Improve task switching performance on unbuffered channels (#21120)

Improve task switching performance on unbuffered channels

* some fixes for compiling the runtime code as c++

* reenable Core+Base ambiguity detection (#20986)

* Resolve ambiguity between cat (git-cat-file and other) methods.
* Reenable the Core+Base ambiguity detection test.

* improve `precompile` mechanism a bit

- improve static_show to readably print a much larger set of types
- add TRACE_COMPILE option to print `precompile` calls

* Restrict mapslices to only reuse StridedArray slices (#21133)

Fix #21123.

* fix #21147, lowering of keyword args with chained static parameters

* rename Core.BottomType => Core.TypeofBottom (#21057)

The old name sounds like an alias for `Bottom = Union{}` rather
than `Type{Bottom} == typeof(Bottom)`. (This last should hold
but currently doesn't – see #21016.)

* fix part of #21016, specificity of `TypeofBottom`

* add `serialize` for `Bottom` to ensure no `Type{<:T}` methods override it

* fix a method cache matching issue with Type{T}

This is needed to avoid confusing `Type{A{B}}` and `Type{A{B} where B}`.
In the future, this can be improved by not using `Type{ }` to dispatch
types with free variables.

* preserve identities of type wrappers in serializer. fixes #20324

* Run test/ambiguous.jl first

works around #21160, hang in test that started happening
when #20986 was merged

* fix other part of #21147, optional instead of keyword args

* Fix mapreduce_impl() for 1-element range (#19325)

* fix mapreduce_impl() for 1-element range

* mapreduce_impl() type stability tests

* wrap long lines

* mapreduce_impl(): add @inbounds

ifirst:ilast should be a valid A indices range (checked by the caller)

* reduce code duplication in _mapreduce()

- let mapreduce_impl() handle all but empty collection cases
- don't test mapreduce_impl() directly
- extend mapreduce() tests with empty/long collections

* mapreduce_impl(f, min/max): don't skip NaNs

- update mapreduce_impl(f, min/max) to 0.6 behaviour of treating NaNs,
  also stop scanning the array once NaN is detected
- add NaN minimum/maximum() tests for 2-element and long arrays

* mapreduce(f, op): don't apply @inbounds for `f`

* clarify how mapreduce_impl() should be used

* DefaultTestSet tests for test_broken and test_skip (#21086)

* DefaultTestSet tests for test_broken and test_skip

Check that test_broken and test_skip do not result in an exception.
Already fixed in 8982605 - "Rework test framework", but present in
julia-0.5 (see #21008).

* Cleanup: use isa rather than typeof

* Correct anchors in README.md (#21166)

* Move manual anchors in README.md below headlines.

Alternative approach to make GitHub correctly render the `README.md`
headlines by simply moving the manual anchors to below the headlines.
Since current manual anchors are uppercase, this keeps external links valid.

* Remove manual HTML anchors from README.md.

The `README.md` is currently rendered incorrectly on GitHub since it includes
manual HTML anchors for headlines.  However, since several years, GitHub
automatically includes these anchors.  Considering the anchors in the
README.md have been added some six years ago, the GitHub feature is probably
younger.

This removes all the manual anchors, for which all match the exact headline
pattern anyway, except the following which differ slightly:

- `<a name="Editor-Terminal-Setup"/>`
- `<a name="Platform-Specific-Notes"/>`
- `<a name="Required-Build-Tools-External-Libraries"/>`

If referenced from somewher externally, these would need to be updated.

Links within this README to anchors are also corrected.

The automatically generated anchors on GitHub follow the pattern of replacing
all spaces with `-` and everything being lowercase, e.g. `## This is a Title`
automatically gets the anchor equivalent to `<a name="this-is-a-title"/>`
to be referenced in markdown with `[Link to a title](#this-is-a-title)`.

* handle small (<16) arrays by _mapreduce() (#21167)

* deprecate use of cat with LibGit2 objects

* Augment sort.jl documentation with doctests (#18923)

* Fix doc system part of #21016 (#21036)

* Breakage.

* Teach doc system to handle 'where'.

* Add parameters to signature.

* Add test and continue to use Union.

* Fix missing docstrings.

* Address comments.

* use newest Documenter, DocStringExtensions

* Use separate build dirs for mbedtls depending on USE_GPL_LIBS

prevents cmake from complaining when you switch sources for the same build dir

* Fix doctest line numbers

* run code signing script in contrib/prepare_release.sh

if present

* fix #21178, lowering of static params with `<:T` syntax

* fix #20671, slowdown loading SIUnits

This adds a fast path to intersection that helps when we have a type
with lots of parameters that just need to be matched up 1-to-1 with
parameters of another type.

Also fixes a bug in intersection uncovered by this test case.

* Various missing git tests

* Add missing test for mapslices (#21181)

Followup from #21133

* Update arrayops.jl

* ensure ShellExecuteW only gets compiled on windows

rather than depending on DCE to delete this code after inference

* dump: ensure jlcall_api is cleared if we don't load the fptr from the sysimg

* Fix `⊑(a::Const, b::PartialTypeVar)`

For `isa(a, Const) && !isa(b, Const)`, `⊑` assumed `b` to be a type,
leading to an error if `b` was a `PartialTypeVar`. Fixed by using
`widenconst(b)`.

* Use `TypeVar`s in `limit_type_depth` in `Union`s in invariant position

Using `Any` for depth-limited `Union` members is only correct in
covariant position, otherwise a `TypeVar` has be be introduced.

* document x % T (#20759)

* add support for AbstractGitHash

* updated fix for #19892 (FFTW threads initialization) (#21169)

* updated fix for #19892; initialize FFTW threads the first time the planner is called (#21127 incorrectly prevented threads from being used at all)

* add test for #21163

* fix #21172, `a = f(x) = 1`

* handle unicode space characters in `parse` for `Bool`

* fix #21180, regression in `readcsv` with quoted unicode fields

* don't make corrupted copies of singleton objects during AST deserialization

this corruption will now be detected when finalizing the sysimg with an assertion

* Update datafmt.jl

Use `endof` instead of `length` as `colval` expects byte index.

* Add test to for fix

* Correct notation used in the rem/mod docstring (#21198)

* Correct notation used in rem/mod docstring
* Fix build warning about overwritten docstring for rem

* More tests for GitConfig

* improve codegen of `setfield!` with constant integer index

* Fix bug in merge! and add test (#21213)

* Add a test for aborting a rebase (#21209)

* Support Rational and Irrational arguments for exp10 (#21199)

* null pointer checks are unnecessary: API should never return valid pointers with an error (#21174)

* Revert "Add a test for aborting a rebase (#21209)" (#21220)

This reverts commit 46b6099.

* Fix rem/mod doc (#21223)

* Two minitests for show

* Make cor work again for complex input (#21205)

Also fix inconsistency in `cov` of vectors. Fixes #21093

* Fix build with LLVM master. (#21194)

In LLVMFPtoInt signature of convertToInteger now expects a MutableArrayRef.

AttributeSet -> AttributeList

* Add tests for internal fastforward merge methods

* Fix bug in backsolve of sparse hermitian matrices (#21165)

* fix #21168, `f.[1,2,3]` should be an error

* Add methods to add refspecs and tests to view fetchspecs (#21227)

* Add methods to add refspecs and tests to view fetchspecs

* Add docs

* fix explicity typo

and very minor wording tweak

* Tag 0.6.0-pre.beta (#21232)

* Non-ff merge test

* Add a test for GitRemote ctor with refspec

* Tests for cleanup and branch deletion

* More fixes for non-1 arrays

* Remove extraneous x^Val methods and add tests

* allow TTYTerminal streams to be any IO types

* ensure that AST of a constant return function is correct

fix #21175

* Allow newline or semicolon in abstract and primitive defs

* Fix exports of undefined/missing/conditionally defined symbols (#21217)

* Remove Base.Parallel export.
* Remove 'big_str' export from mpfr.jl
* Remove 'dense' from export in sparse.jl

* deprecate unintended methods of zeros, ones (#21183)

* fix error handling for ccall without a concrete layout

fix #21104

* Test that `convert(Tuple{...}, (...))` throws for mismatched numbers of elements

* fix #20614, need error for `Vararg` on non-final argument

* Ensure that `collect(::AbstractArray)` always returns an `Array` (#21257)

The docstring specifies that the return type is `Array`, but `similar` isn't guaranteed to return an `Array`. (Exceptions include AxisArrays, OffsetArrays, etc.)

* fix #21271, codegen of typeassert involving `Tuple{Type{T}}`

* [docathon] Add examples for diff_files

* Add a test for aborting a rebase (#21222)

* Add a test for aborting a rebase

* Fix bad merge tests corrupting test_repo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

5 participants