deprecate unescaped shell special chars in commands #19786
Conversation
StefanKarpinski
added
the
io
|
|
||
| # Julia process with passed in command line flag arguments | ||
| cmd = `cd $dir && $tval $exename $exeflags` | ||
| cmd = `cd $dir '&&' $tval $exename $exeflags` |
There was a problem hiding this comment.
this is in the ssh code, do we test this?
There was a problem hiding this comment.
I'm not sure. @amitmurthy, @tanmaykm?
There was a problem hiding this comment.
might be behind the full-tests env var flag? or remote only...
we could add some simple tests that sending quoted/escaped special chars to programs that should be able to use them still act as expected
There was a problem hiding this comment.
If you wish to test, set JULIA_TESTFULL=1 in your env. Enable password less ssh login into your local machine. Run the parallel test. This will test SSH Manager.
Else I can test this locally in a couple of days.
There was a problem hiding this comment.
addprocs hangs on the open call with this change. Printing cmd just before the open shows that && is not treated correctly.
`ssh -T -a -x -o ClearAllForwardings=yes -n localhost "sh -l -c \"cd /Users/amitm/Julia/julia '&&' /Users/amitm/Julia/julia/usr/bin/julia --worker ehqbRgqspml7nZLg\""`
fb1fa13 to
fc61be6
Compare
StefanKarpinski
force-pushed
the
sk/shellspecial
branch
from
fc61be6 to
3abeb79
Compare
|
Ok, @amitmurthy – this seems to work for me locally although the output from |
|
|
||
| # launch the remote Julia process | ||
|
|
||
| # detach launches the command in a new process group, allowing it to outlive | ||
| # the initial julia process (Ctrl-C and teardown methods are handled through messages) | ||
| # for the launched porcesses. | ||
| @show cmd |
|
Yeah, the output prints a few expected errors and one of the reasons it is not part of mainstream CI. Test passes locally for me too. Is the Travis error related to this? A comment/docstring explaining the use of |
StefanKarpinski
force-pushed
the
sk/shellspecial
branch
from
3abeb79 to
4c271fc
Compare
|
The Travis failure is unrelated since I'm seeing it in multiple ongoing PRs. Something recently merged broke 32-bit Linux. |
|
agreed it's going to be a bit hard to follow when this needs to be bypassed |
|
It's not an exported function. I nevertheless added a docstring. If we have a way of representing pipelines like |
|
Should these be getting passed as strings instead then in this kind of situation? |
|
They could, but that doesn't really address the issue, which is that we don't currently have a way to represent command pipelines. Until we do, which requires a deprecation release cycle, anything we do is a hack. I could change the default for |
|
Let me summarize:
|
|
Sure. In this particular case though, the command in question is used as an argument to sh, so unless we implement a full sh parser and semantics, a Julia Cmd isn't the right type for holding argument data. |
|
We only need enough shell syntax to express the pipeline that's used here, which I do intend to do. If more than that is required, then someone should implement whatever API they want in a package. |
|
Something got messed up here – either I forget to add some of the changes or I just messed this up. More changes are needed and are on their way here. |
|
This should leave a todo note in deprecated.jl. |
|
About what? |
|
That the deprecation should be removed so it doesn't get forgotten. This kind of came out of nowhere in #19598 without a lot of discussion about the future plan, so it should be tracked one way or another. |
|
The special characters are going to mean something in 1.0 – namely they're going to construct pipelines, etc. There has been discussion about it previously. No need for a note since this is going to have a new meaning. |
Not that I can recall, any more recently than |
|
I've definitely discussed with Jeff, Keno and Jameson but maybe not on GitHub. Issue: #20401. |
No description provided.