Issue executing a command within a directory #719

Closed
rahul-sekhar opened this Issue Oct 19, 2013 · 15 comments

Comments

Projects
None yet
9 participants
@rahul-sekhar

While writing a task, this works fine and executes within "/some/path":

within "/some/path" do
    execute "pwd"
end

This does not - it executes within the deploy_to path:

within release_path do
    execute "pwd && ls"
end

In fact, it seems like executing a command with whitespace in it does not work as expected in a within block.

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Oct 19, 2013

Member

Please see the SSHKit (the cap backend driver). There's no way to apply
within, with, as, etc to string commands, as the string might be anything.

The command must be formatted as per the examples in SSHkit's EXAMPLES.md

On Saturday, October 19, 2013, Rahul Sekhar wrote:

While writing a task, this works fine and executes within "/some/path":

within "/some/path" do
execute "pwd"
end

This does not - it executes within the deploy_to path:

within release_path do
execute "pwd && ls"
end

In fact, it seems like executing a command with whitespace in it does not
work as expected in a within block.


Reply to this email directly or view it on GitHubhttps://github.com/capistrano/capistrano/issues/719
.

Lee Hambley

http://lee.hambley.name/
+49 (0) 170 298 5667

Member

leehambley commented Oct 19, 2013

Please see the SSHKit (the cap backend driver). There's no way to apply
within, with, as, etc to string commands, as the string might be anything.

The command must be formatted as per the examples in SSHkit's EXAMPLES.md

On Saturday, October 19, 2013, Rahul Sekhar wrote:

While writing a task, this works fine and executes within "/some/path":

within "/some/path" do
execute "pwd"
end

This does not - it executes within the deploy_to path:

within release_path do
execute "pwd && ls"
end

In fact, it seems like executing a command with whitespace in it does not
work as expected in a within block.


Reply to this email directly or view it on GitHubhttps://github.com/capistrano/capistrano/issues/719
.

Lee Hambley

http://lee.hambley.name/
+49 (0) 170 298 5667

@freemanoid

This comment has been minimized.

Show comment
Hide comment
@freemanoid

freemanoid Oct 23, 2013

Contributor

So we should use it like this:

within release_path do
    execute "pwd"
    execute "ls"
end

@leehambley right?

Contributor

freemanoid commented Oct 23, 2013

So we should use it like this:

within release_path do
    execute "pwd"
    execute "ls"
end

@leehambley right?

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Oct 23, 2013

Member

Unless you have a good reason not to do so, yes. If you write commands with spaces in their names the complexity for correct shell escaping goes through the roof, imagine the example:

within release_path do
    execute "rm -rf some file"
end

Its impossible to know which spaces should be escaped (probably the user meant to type some\ file, or 'some file'), and which should not (the spaces around the -rf)

For that reason, if people wish to use spaces in their commands, they are free to do something like this:

execute <<-EOCOMMAND
  (
    cd #{release_path};
    pwd && ls;
  )
EOCOMMAND

We (the core team) are of the opinion that our decision makes sense, although admittedly from outside it's not immediately clear the reasoning behind that.

Member

leehambley commented Oct 23, 2013

Unless you have a good reason not to do so, yes. If you write commands with spaces in their names the complexity for correct shell escaping goes through the roof, imagine the example:

within release_path do
    execute "rm -rf some file"
end

Its impossible to know which spaces should be escaped (probably the user meant to type some\ file, or 'some file'), and which should not (the spaces around the -rf)

For that reason, if people wish to use spaces in their commands, they are free to do something like this:

execute <<-EOCOMMAND
  (
    cd #{release_path};
    pwd && ls;
  )
EOCOMMAND

We (the core team) are of the opinion that our decision makes sense, although admittedly from outside it's not immediately clear the reasoning behind that.

@leehambley leehambley closed this Oct 23, 2013

@pandora2000

This comment has been minimized.

Show comment
Hide comment
@pandora2000

pandora2000 Nov 13, 2013

@freemanoid Or you can do:

within release_path do
  execute "rm", "-rf some file"
end

This is because should_map? in command.rb ( SSHKit ) interrupt to_command to make 'within' construction ( adding 'cd foo && ' ) .

@freemanoid Or you can do:

within release_path do
  execute "rm", "-rf some file"
end

This is because should_map? in command.rb ( SSHKit ) interrupt to_command to make 'within' construction ( adding 'cd foo && ' ) .

amatriain added a commit to amatriain/feedbunch that referenced this issue Dec 7, 2013

Fixed God-related capistrano tasks.
Instead of manually running a "cd" command, "within" blocks are used.

Instead of manually setting the RESQUE_ENV environment variable when running commands, "with" blocks are used (and RAILS_ENV is added to the default environment in config/deploy/production.rb and staging.rb files, so it is used in all commands).

Instead of manually running "bundle exec god" every time, a :god key is added to the SSHKit command map so that the :god command can be executed by capistrano and actually a "bundle exec god" is executed in the remote servers.

Most of these are features of SSHKit, which is the library that powers Capistrano 3:

	https://github.com/capistrano/sshkit

An important limitation of SSHKit is that "execute" cannot be passed a command string with whitespaces in it. Instead several strings (without whitespaces) can be passed, each one with part of the whole command, and when running it in the remote servers SSHKit concatenates all of them with witespaces between them. It makes for an awkward syntax but devs seem to think it unavoidable. For more details see:

	capistrano/capistrano#719
@andyzei

This comment has been minimized.

Show comment
Hide comment
@andyzei

andyzei Mar 2, 2014

Sorry, as someone who just picked up Cap3 today, this is kind of nutty.

The very first thing you're going to do when learning capistrano is to try and restart your server after getting your code deployed. It took me about 3 hours to debug why the code below executes under release_path

within release_path do
    execute "ls"
end

while

within release_path do
    execute "ls -la"
end

executes under $HOME

Is there a good reason why the latter example shouldn't throw an error, as it's not going to do what it looks like it's going to do?

I've looked at the SSHKit examples. They are useful, but while there are some nice gems in there, it's really, really, hard to try and derive the DSL that they're using. I'm not a Rake expert, but I'm reasonably competent with UNIX. I'm guessing that there are other people like me trying to use capistrano.

I have a fork all ready to go for a README update, but really, I have no clue what to write, other than @pandora2000 or @leehambley s' suggestions above.

For example, one of the SSHKit examples:

on hosts do |host|
  as user: 'www-data', group: 'project-group' do
    within '/var/log' do
      execute :touch, 'somefile'
      execute :ls, '-l'
    end
  end
end

Really, really, stupid question: how is the :touch symbol defined, what's its path, what's its env, and why is that syntax preferred over "execute 'touch, 'somefile'"?

Thanks,

Andy

andyzei commented Mar 2, 2014

Sorry, as someone who just picked up Cap3 today, this is kind of nutty.

The very first thing you're going to do when learning capistrano is to try and restart your server after getting your code deployed. It took me about 3 hours to debug why the code below executes under release_path

within release_path do
    execute "ls"
end

while

within release_path do
    execute "ls -la"
end

executes under $HOME

Is there a good reason why the latter example shouldn't throw an error, as it's not going to do what it looks like it's going to do?

I've looked at the SSHKit examples. They are useful, but while there are some nice gems in there, it's really, really, hard to try and derive the DSL that they're using. I'm not a Rake expert, but I'm reasonably competent with UNIX. I'm guessing that there are other people like me trying to use capistrano.

I have a fork all ready to go for a README update, but really, I have no clue what to write, other than @pandora2000 or @leehambley s' suggestions above.

For example, one of the SSHKit examples:

on hosts do |host|
  as user: 'www-data', group: 'project-group' do
    within '/var/log' do
      execute :touch, 'somefile'
      execute :ls, '-l'
    end
  end
end

Really, really, stupid question: how is the :touch symbol defined, what's its path, what's its env, and why is that syntax preferred over "execute 'touch, 'somefile'"?

Thanks,

Andy

@pandora2000

This comment has been minimized.

Show comment
Hide comment
@pandora2000

pandora2000 Mar 2, 2014

I think there is no 'good' reason, but a problem of implementation.
I forgot the detail, but the reason

within release_path do
    execute "ls -la"
end

doesn't succeed is the implementation set a prefix (in this case, cd to releath_path) to a command only when the command has no space. Some code do this, and this should be improved I think.

Symbol :touch means nothing but 'touch' unless you replace it with

SSHKit.config.command_map[:rake] = "/usr/local/rbenv/shims/rake"

I think there is no 'good' reason, but a problem of implementation.
I forgot the detail, but the reason

within release_path do
    execute "ls -la"
end

doesn't succeed is the implementation set a prefix (in this case, cd to releath_path) to a command only when the command has no space. Some code do this, and this should be improved I think.

Symbol :touch means nothing but 'touch' unless you replace it with

SSHKit.config.command_map[:rake] = "/usr/local/rbenv/shims/rake"
@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Mar 2, 2014

Member

For whatever it's worth about 2 weeks ago large new sections were added to docs, and README explaining this. There's a good reason, and it's throughly documented and discussed here, at the mailing list, and importantly documented.

Member

leehambley commented Mar 2, 2014

For whatever it's worth about 2 weeks ago large new sections were added to docs, and README explaining this. There's a good reason, and it's throughly documented and discussed here, at the mailing list, and importantly documented.

@damphyr

This comment has been minimized.

Show comment
Hide comment
@damphyr

damphyr Mar 3, 2014

Just as @leehambley said, there is a good reason.The reason has to do with proper shell escaping and how it is damn near impossible to do for generic strings. It's even on this ticket right at the top.

This is mostly SSHKit territory, has nothing to do with rake or tasks, so look up the command map documentation and examples in the SSHKit documentation. It has been expanded and clarified quite recently.

damphyr commented Mar 3, 2014

Just as @leehambley said, there is a good reason.The reason has to do with proper shell escaping and how it is damn near impossible to do for generic strings. It's even on this ticket right at the top.

This is mostly SSHKit territory, has nothing to do with rake or tasks, so look up the command map documentation and examples in the SSHKit documentation. It has been expanded and clarified quite recently.

@sarink

This comment has been minimized.

Show comment
Hide comment
@sarink

sarink Jan 20, 2015

+1 to everything @andyzei said

I don't understand why you wouldn't just execute the command, as-is. If the user forgot to escape their spaces, so what? That doesn't seem like capistrano's problem, nor is it capistrano's problem to protect a user from themselves. Bash doesn't care if you mistype a bash command, why would capistrano? So this "safety feature" really results in an awfully inconsistent/difficult-to-follow API, imo.

sarink commented Jan 20, 2015

+1 to everything @andyzei said

I don't understand why you wouldn't just execute the command, as-is. If the user forgot to escape their spaces, so what? That doesn't seem like capistrano's problem, nor is it capistrano's problem to protect a user from themselves. Bash doesn't care if you mistype a bash command, why would capistrano? So this "safety feature" really results in an awfully inconsistent/difficult-to-follow API, imo.

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Jan 21, 2015

Member

You are free to use any other piece of software that more closely aligns
with your philosophy.

When we have bugs we fix them, when features or caveats are difficult to
use or unclear, we improve the documentation, this is how OSS should work,
and you at also free to contribute code, and put in the work to make a case
for changing something.

Sent from my Nexus 5.
On 20 Jan 2015 22:29, "Kabir Sarin" notifications@github.com wrote:

+1 to everything @andyzei https://github.com/andyzei said

I don't understand why you wouldn't just execute the command, as-is. If
the user forgot to escape their spaces, so what? That doesn't seem like
capistrano's problem, nor is it capistrano's problem to protect a user from
themselves. Bash doesn't care if you mistype a bash command, why would
capistrano? So this "safety feature" really results in an awfully
inconsistent/difficult-to-follow API, imo.


Reply to this email directly or view it on GitHub
#719 (comment)
.

Member

leehambley commented Jan 21, 2015

You are free to use any other piece of software that more closely aligns
with your philosophy.

When we have bugs we fix them, when features or caveats are difficult to
use or unclear, we improve the documentation, this is how OSS should work,
and you at also free to contribute code, and put in the work to make a case
for changing something.

Sent from my Nexus 5.
On 20 Jan 2015 22:29, "Kabir Sarin" notifications@github.com wrote:

+1 to everything @andyzei https://github.com/andyzei said

I don't understand why you wouldn't just execute the command, as-is. If
the user forgot to escape their spaces, so what? That doesn't seem like
capistrano's problem, nor is it capistrano's problem to protect a user from
themselves. Bash doesn't care if you mistype a bash command, why would
capistrano? So this "safety feature" really results in an awfully
inconsistent/difficult-to-follow API, imo.


Reply to this email directly or view it on GitHub
#719 (comment)
.

@sarink

This comment has been minimized.

Show comment
Hide comment
@sarink

sarink Jan 21, 2015

I mean, is there any practical reason other than trying to protect a user from themselves being bad at bash? Are there other disastrous consequences that could stem from this? Or...something else?

My post was not meant to be offensive, I was (and still am) simply wondering why you believe this way is better.

sarink commented Jan 21, 2015

I mean, is there any practical reason other than trying to protect a user from themselves being bad at bash? Are there other disastrous consequences that could stem from this? Or...something else?

My post was not meant to be offensive, I was (and still am) simply wondering why you believe this way is better.

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Jan 22, 2015

Member

My post was not meant to be offensive, I was (and still am) simply wondering why you believe this way is better.

Years of watching people screw things up... we had to make a decision, and the command map function is valuable enough to justify making this slightly (for some experienced people) weird API. IT's not optimal, by any means, but given how cd and env vars work in short lived SSH sessions, we don't have a lot of choice. Also, we tried using SSH back channels to manage the connection state (directory, paths, env vars, etc) but it created a massive complexity and synchronisation burden.

Member

leehambley commented Jan 22, 2015

My post was not meant to be offensive, I was (and still am) simply wondering why you believe this way is better.

Years of watching people screw things up... we had to make a decision, and the command map function is valuable enough to justify making this slightly (for some experienced people) weird API. IT's not optimal, by any means, but given how cd and env vars work in short lived SSH sessions, we don't have a lot of choice. Also, we tried using SSH back channels to manage the connection state (directory, paths, env vars, etc) but it created a massive complexity and synchronisation burden.

@scottsb

This comment has been minimized.

Show comment
Hide comment
@scottsb

scottsb Mar 8, 2015

@leehambley I completely follow the reasoning for why this behaves as it does, but I'm wondering: given the "inconsistency" and the fact the right way 95% of the time is to set the command without white space, would you consider logging a notice to the console if a command is included with whitespace in it? There would naturally need to be some way to disable the notice for those cases where someone knows what they're doing.

scottsb commented Mar 8, 2015

@leehambley I completely follow the reasoning for why this behaves as it does, but I'm wondering: given the "inconsistency" and the fact the right way 95% of the time is to set the command without white space, would you consider logging a notice to the console if a command is included with whitespace in it? There would naturally need to be some way to disable the notice for those cases where someone knows what they're doing.

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Mar 8, 2015

Member

The problem is both are equally valid ways to use the APIs, so warning on
one of two equally useful use cases seems inelegant.
On 8 Mar 2015 03:25, "Scott Buchanan" notifications@github.com wrote:

@leehambley https://github.com/leehambley I completely follow the
reasoning for why this behaves as it does, but I'm wondering: given that
the behavior is inconsistent and the "right" way is to have the command
without white space, would you consider logging a notice to the console if
a command is included with whitespace in it? There would naturally need to
be some way to disable the notice for those cases where someone knows what
they're doing.


Reply to this email directly or view it on GitHub
#719 (comment)
.

Member

leehambley commented Mar 8, 2015

The problem is both are equally valid ways to use the APIs, so warning on
one of two equally useful use cases seems inelegant.
On 8 Mar 2015 03:25, "Scott Buchanan" notifications@github.com wrote:

@leehambley https://github.com/leehambley I completely follow the
reasoning for why this behaves as it does, but I'm wondering: given that
the behavior is inconsistent and the "right" way is to have the command
without white space, would you consider logging a notice to the console if
a command is included with whitespace in it? There would naturally need to
be some way to disable the notice for those cases where someone knows what
they're doing.


Reply to this email directly or view it on GitHub
#719 (comment)
.

@codener

This comment has been minimized.

Show comment
Hide comment
@codener

codener May 13, 2015

This is the same behavior as Ruby's system, exec etc methods have, right? I think the only unexpected thing is that a command with spaces is actually run, but only ignoring path settings.

codener commented May 13, 2015

This is the same behavior as Ruby's system, exec etc methods have, right? I think the only unexpected thing is that a command with spaces is actually run, but only ignoring path settings.

@capistrano capistrano locked and limited conversation to collaborators May 13, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.