Implement `RubyUtils.find_executable`.

[noniq]

First try on implementing #102. Not sure about the module name RubyUtils, though – happy to take suggestions for alternative names!

TODO: Do we need to do something special to support RVM?

[sorbits]

@jacob-carlborg, @sanssecours Any input on this one?

(my own knowledge of the ruby “tool chain” and the multitude of ways to execute it is rather limited)

[jacob-carlborg]

I'll take a look. Is intended to be used by other bundles, in that case how?

[noniq]

I plan on using it in the RSpec and RuboCop bundles. Both are now implementing a quite similar logic on their own. Would also make sense to use it for #99 for example. (I think quite a large part of that issue’s discussion could have been avoided if such a feature had already been present :-)

[jacob-carlborg]

Well, how do you actually require a file from another bundle?

[noniq]

Allan mentioned this once on the mailing list, it’s also described here: http://manual.textmate.org/bundles.html#requirements :-)

(I already have an experimental branch in the RuboCop bundle relying on this – and the presence of this functionality in bundle support the Ruby bundle, of course –, works like a charm.)

(Updated to correct the last sentence: I meant to refer to the Ruby bundle of course, not bundle support.)

[jacob-carlborg]

Ah, I see.

[jacob-carlborg]

TODO: Do we need to do something special to support RVM?

Yes, something. I'm not sure what. It's easy to do from a shell script but I'm not sure how to do it from within Ruby.

[sorbits]

We can add a “project scope” for projects with a Gemfile […] How would that work?

In .tm_properties you can presently do this:

FOO = 'no-make'

[ attr.project.make ]
FOO = "make"

Now try echo $FOO in a document in a project with a Makefile and one without, and you’ll see different values.

The primary use of this is key bindings, e.g. ⌘Y shows the proper SCM actions and ⌘B invokes the proper build command…

Not having it trump everything means that if a user reports that for some project, our logic is wrong, we cannot provide them with any workaround Not sure I understand. By default it would use what's in the Gemfile, the user has already control of the Gemfile.

And the Gemfile is always correct?

For example is it unthinkable that user has a project being run both in Vagrant and on macOS, the Gemfile.lock file would be shared between the two, but something could prevent it from executing w/o issues on the Mac?

As I said initially, my knowledge of the ruby “tool chain” is very limited, but I think it’s standard practice to have the environment variable trump everything.

So I’d rather ask you, why do you set an environment variable for a fallback option instead of adding the location of the fallback executable to PATH?

[noniq]

Using it as a fallback, IMHO that is the job of the PATH variable. I.e. ensure that whatever fallback executable you want to use when there is no TM_«executable», no Gemfile, no etc., can be found via PATH.

I think that’s a compelling argument. And it’s simple to explain, too: “Setup PATH so that it includes all the executables you want to use by default, use TM_«foo» for project-specific overrides.”

If you would add a .tm_properties file, why not a Gemfile?

In shared projects it may not be possible to change the Gemfile just because of personal preferences / configuration choices. Adding a .tm_properties file (either Git-ignored or one directory above the Git root) seems more suitable.

[jacob-carlborg]

In .tm_properties you can presently do this: FOO = 'no-make' [ attr.project.make ] FOO = "make" Now try echo $FOO in a document in a project with a Makefile and one without, and you’ll see different values.

Hmm, then I would need to set something like TM_RUBOCOP = 'bundle exec rubocop', which is not compatible with the current code in this PR, File.executable?(ENV[env_var]). I'm also not sure if the system will interpret the whole bundle exec rubocop as one single command or as one command with two arguments.

For example is it unthinkable that user has a project being run both in Vagrant and on macOS, the Gemfile.lock file would be shared between the two, but something could prevent it from executing w/o issues on the Mac?

That's a possibility. I would say that the Gemfile is broken in that case.

So I’d rather ask you, why do you set an environment variable for a fallback option instead of adding the location of the fallback executable to PATH?

I don't know. Maybe because the documentation for most bundles say that the TM_... environment variable should be set.

[jacob-carlborg]

I guess I could try adding the necessary executables to PATH instead.

[jacob-carlborg]

Would it make sense to add an exectuable script that can be used from other languages than Ruby?

[sanssecours]

Not sure about the module name RubyUtils, though – happy to take suggestions for alternative names!

How about something like Executable.find or RubyCommand.find instead of RubyUtils.find_executable?

TODO: Do we need to do something special to support RVM?

Yes, something. I'm not sure what. It's easy to do from a shell script but I'm not sure how to do it from within Ruby.

In a shell script you usually just source "$HOME/.rvm/scripts/rvm" to support RVM. I did not find anything about supporting RVM inside a Ruby script though. One super ugly hack to add this support: Run a Bash script inside the Ruby script. For example, the following command prints the path to the RVM ruby executable:

puts `bash -c '[[ -f "$HOME/.rvm/scripts/rvm" ]]; \
                 . "$HOME/.rvm/scripts/rvm"; 
               which ruby'`

By the way: I ported “Reformat Document” (Issue #99) to Ruby on my machine:

#!usr/bin/ruby

require 'shellwords'
require ENV['TM_BUNDLE_SUPPORT'] + '/lib/ruby_utils'
require ENV['TM_SUPPORT_PATH'] + '/lib/tm/detach'

Dir.chdir(ENV['TM_PROJECT_DIRECTORY'] || File.dirname(ENV['TM_FILEPATH'].to_s))

rubocop = RubyUtils.find_executable('rubocop')
unless rubocop
  error_message = 'This command requires RuboCop. ' \
                  'Please install RuboCop via “gem install rubocop”.'
  `"$DIALOG" tooltip --text "#{error_message}"`
end
rubocop = rubocop.join ' '

aha = `which aha`.rstrip
if aha
  color = 'n'
  output_format = '--html'
else
  output_format = '--text'
end

TextMate.detach do
  output = `#{rubocop} -a#{color.to_s} "$TM_FILEPATH" #{'| aha' if aha}`
  `"$DIALOG" tooltip #{output_format} #{output.shellescape}`
end

It works for my purposes, since I only use rbenv (and neither RVM or Bundler). I also tested the Bundler support which works too.

Hmm, then I would need to set something like TM_RUBOCOP = 'bundle exec rubocop', which is not compatible with the current code in this PR, File.executable?(ENV[env_var]).

Looks like we should remove the check above, especially since RubyUtils.find_executable('rubocop') might return something like ['bundle', 'exec', 'rubocop'], if the current project contains a Gemfile.

[sorbits]

I like the simpler Executable.find API.

As for TM_RUBOCOP = 'bundle exec rubocop' I think this should be supported.

So I guess the check needs to be a little more complex, e.g.:

def is_shell_executable?(command, search = ENV['PATH'].to_s)
  if exe = command.to_s.shellsplit.first
    search.split(':').any? do |dir|
      File.executable?(File.expand_path(exe, dir))
    end
  else
    false
  end
end

Or should we just assume that if the variable is set, then it should be used, as ignoring misconfigured values might be doing the user a disservice.

[jacob-carlborg]

Or should we just assume that if the variable is set, then it should be used, as ignoring misconfigured values might be doing the user a disservice.

I think so.

[noniq]

Executable.find sounds nice. Does not refer specifically to Ruby executables, though – not sure if this could lead to misunderstandings? (On the other hand, apart from the Gemfile part, the functionality is Ruby-agnostic anyway [which, on the other other hand, may lead to the question if TextMate should have a generic find_executable API in bundle support?])

Anyway, I’m fine with both Executable.find and RubyExecutable.find.

I’ll update the code to support space-separated values in the TM environment vars and to do no explicit checking of the env var values. And I’ll look deeper into RVM and try to come up with a solution.

[sorbits]

For general API: What other things could you see using this?

TextMate already has built-in support for requiredCommands, so if there is something re-usable about this ruby stuff, then probably we should rather update TextMate’s native support (since that does provide a better user experience).

As this API is in the Ruby bundle, I think that it’s “only for” Ruby is implied.

On 30 Aug 2016, at 18:08, Stefan Daschek wrote:

Executable.find sounds nice. Does not refer specifically to Ruby executables, though – not sure if this could lead to misunderstandings? (On the other hand, apart from the Gemfile part, the functionality is Ruby-agnostic anyway [which, on the other other hand, may lead to the question if TextMate should have a generic find_executable API in bundle support?])

Anyway, I’m fine with both Executable.find and RubyExecutable.find.

I’ll update the code to support space-separated values in the TM environment vars and to do no explicit checking of the env var values. And I’ll look deeper into RVM and try to come up with a solution.

You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/textmate/ruby.tmbundle/pull/104#issuecomment-243491951

[sanssecours]

…as ignoring misconfigured values might be doing the user a disservice.

A simple check – like the one Allan wrote – for the correct value of an environment variable such as TM_RUBOCOP would be nice. I think in the case of an incorrect value RubyUtils.find_executable could just show a window via "$DIALOG" alert – containing an error message about the incorrect value of the environment variable – and then exit. This way, the user of the API does not need to care about incorrect values of environment variables.

It might even make sense to guarantee that if RubyUtils.find_executable returns a command (and not nil), that this command is executable. After all, the “executable” is right there in the name.

[noniq]

I think in the case of an incorrect value RubyUtils.find_executable could just show a window via "$DIALOG" alert – containing an error message about the incorrect value of the environment variable – and then exit.

I think handling the “executable not found” case belongs to the caller. For example, in the RuboCop bundle I want to do handle this differently based on whether the command was invoked implicitly via did-save callback (show an unintrusive tooltip) or explicitly (show an alert with detailled error message).

It might even make sense to guarantee that if RubyUtils.find_executable returns a command (and not nil), that this command is executable. After all, the “executable” is right there in the name.

That was my original intention. I’m not sure how to do this though if we consider cases like bundle exec foobar. Especially I do not want to really invoke the command only to see if it’s present and executable – some commands might need an non-negligible startup time (for example bundle exec rspec in a Rails project with a big Gemfile).

[sanssecours]

I think handling the “executable not found” case belongs to the caller.

You are right, but then it would be nice if the function could provide more information about the error case. Otherwise the caller has to – more or less – do the same checks as the function, if he wants to provide the user of the TextMate command with a helpful error message. I think it would be best if the command would just raise different exceptions in the error case. Then the caller could even use the message of the exception as default error message for a TextMate command.

That was my original intention. I’m not sure how to do this though if we consider cases like bundle exec foobar.

I think the simple check which just looks at the first part of the command – i.e. File.executable?('bundle') in the case of bundle exec foobar – is enough. After all, if bundle is executable, then so is bundle exec foobar.

[noniq]

I think it would be best if the command would just raise different exceptions in the error case.

Hm, which different exceptions could that be (apart from “executable not found“)?

I think the simple check which just looks at the first part of the command – i.e. File.executable?('bundle') in the case of bundle exec foobar – is enough. After all, if bundle is executable, then so is bundle exec foobar.

Not sure: bundle exec foobar could still fail if foobar is not installed. Remember that this is the codepath where we’re looking at ENV["TM_FOOBAR"], so we should not make any assumptions about Gemfiles etc. (There is a similar problem with rbenv: Rbenv shims are always in the search path, even if a gem isn’t installed for the current Ruby version. That’s why I implemented this additional check. But it works for the specific case of rbenv shims only.)

[jacob-carlborg]

I think that the method not should try to be too cleaver, that will most likely cause issues because it's used in some unexpected way. If ENV["TM_FOOBAR"] is set, just execute that, it's the user that sets it anyway.

[sanssecours]

Hm, which different exceptions could that be (apart from “executable not found“)?

Something like EnvironmentVariableNotExecutableError together with an error message might be nice. For example, if the value of the environment variable is not executable we could raise EnvironmentVariableNotExecutableError providing an error message such as:

The value of the environment variable ”TM_RUBOCOP“: 

     “does_not_exist/rubocop”

does not specify a valid executable command.

Not sure: bundle exec foobar could still fail if foobar is not installed.

That is of course true, but calling some command can always fail. If we use the simple check for executability, then the caller can always be sure that he is able to call the returned command. After running the command, he can then check the exit code to verify that everything went fine.

There is a similar problem with rbenv: Rbenv shims are always in the search path, even if a gem isn’t installed for the current Ruby version. That’s why I implemented this additional check. But it works for the specific case of rbenv shims only.

I think the more checks the API provides, the nicer it is to use. Of course we can not check everything before we run the command, but basic executability should be doable.

[noniq]

I like the idea of providing more info about the cause if the executable wasn’t found, so the method now raises an error with a (hopefully) helpful error message in these cases.

Also renamed the method to Executable.find, fixed a bug where env var settings would not work for executables in PATH, and made sure that env var settings like bundle exec foobar work correctly.

To do: Investigate RVM.

[jacob-carlborg]

@noniq the Ruby on Rails bundle has a "Set RVM..." command, that might be interesting to take a look at.

The Set RVM... command just edits the .rvmrc file. Not interesting for our purpose.

[jacob-carlborg]

@noniq I might have a solution for RVM, the do action of the rvm command. From the documentation:

## Usage
    $ rvm [all|all-gemsets|<ruby>,...|<path>] [--verbose|--summary|--yaml|--json] do <command> ...
    $ rvm in <path> do <some-command> ...
Executes arbitrary commands against given a set of rvm environments

So what you can do is something like this rvm #{ENV['TM_PROJECT_DIRECTORY'} do <command>. To execute the command in the given directory, prefix the path with in, like this:

rvm in #{ENV['TM_PROJECT_DIRECTORY']} do <command>

Use the full path to the RVM binary: ~/.rvm/bin/rvm.

[noniq]

Great, thanks for investigating!

I wonder: Should we test for the existence of ~/.rvm/bin/rmv and use this, or should we check if rvm is in the path (assuming that someone who wants RVM to be used has set up their $PATH in TextMate accordingly)?

[jacob-carlborg]

We need to test if ~/.rvm/bin/rvm exits. If RVM is setup properly it will not be in the PATH, because the rvm command will be a Bash function and not an executable. And it needs to have the RVM file in the project directory, if not this error will occur:

Could not determine which Ruby to use; . should contain .rvmrc or .versions.conf or
.ruby-version or .rbfu-version or .rbenv-version, or an appropriate line in Gemfile.
( see: 'rvm usage' )

[noniq]

As far as I understand (see https://rvm.io/workflow/scripting), RVM is both an exectuable in path and a shell function.

I was thinking along the lines of “If you want to use something non-standard from within TextMate, make sure to setup TextMate’s $PATH accordingly.” (And it would simplify the code a bit becaue we wouldn’t need to check for both a user and a system wide install of RVM). But not sure how common that thinking is, that’s why I asked on the mailing list, to hopefully get input from people actually using RVM :-)

Apart from this minor detail, I think what the code then needs to do is:

• Use rvm do . which «executable» to check if the executable was installed for the given Ruby version.
• If yes, return %w(rvm do . #{executable}) as invocation to run this executable.

Did I miss something?

[jacob-carlborg]

As far as I understand (see https://rvm.io/workflow/scripting), RVM is both an exectuable in path and a shell function.

Yes, but the binary doesn't have all the features as the function. When RVM is used from the command line and it's always used as a function.

When RVM is used from within TextMate, the TM_RUBY variable should be set to ~/.rvm/bin/rvm-auto-ruby which will automatically switch to the correct version based on the current directory [1]. At least I have not had a reason to add RVM to the PATH, because the rvm command itself it not invoked, it's the rvm-auto-ruby which is a wrapper for the ruby command that is invoked.

But not sure how common that thinking is, that’s why I asked on the mailing list, to hopefully get input from people actually using RVM :-)

I am using RVM with TextMate 😉.

Use rvm do . which «executable» to check if the executable was installed for the given Ruby version.

The path comes before the do action: rvm . do which «executable»

Did I miss something?

You need to check for the RVM file in the project directory. If there is no such file the above RVM command will fail.

[1] https://rvm.io/integration/textmate

[noniq]

Great, thanks! (I was under the impression that you’re using rbenv, no idea how I got this mixed up, sorry). Will update the PR accordingly this evening.

[noniq]

@jacob-carlborg I added RVM support, but I could test it only superficially. Does it work for you?

[jacob-carlborg]

Why attic in the Support/test directory?

[noniq]

Why attic in the Support/test directory?

Couldn’t come up with a better name – open for suggestions :-)

[jacob-carlborg]

Couldn’t come up with a better name – open for suggestions :-)

Integration?

[noniq]

“Integration” sounds like this folder would contain integration tests. But instead it contains test data structures – maybe we should call it “fixtures”?

[sanssecours]

But instead it contains test data structures – maybe we should call it “fixtures”?

How about data then? This would leave us with the path test/data, which sounds about right, according to your description.

By the way: I think you should use the directory Tests instead of Support/test (Source: Bundle Style Guide – Section “Test Files”).

[jacob-carlborg]

“Integration” sounds like this folder would contain integration tests. But instead it contains test data structures – maybe we should call it “fixtures”?

Ah, the test itself is not located in the directory. My mistake. I think data sounds good.

[jacob-carlborg]

By the way: I think you should use the directory Tests instead of Support/test (Source: Bundle Style Guide – Section “Test Files”).

Please no. I see the structure in the Support directory as the structure of a Ruby gem. The conventsions for gems is to have a directory called test (or spec if RSpec is being used).

[noniq]

Updated again. Regarding directory naming and structure I decided to go with fixtures and keep the tests directory under Support (as @jacob-carlborg mentioned, this better mirrors the structure of most Ruby libs).

RVM support is now baked into all the “normal” cases (except environment vars, see commit message), and RVM project file detection should work more reliable now.

[sanssecours]

Regarding directory naming and structure I decided to go with fixtures and keep the tests directory under Support (as @jacob-carlborg mentioned, this better mirrors the structure of most Ruby libs).

Sounds good, although I still think the name fixture is not that great. Here is what Dictionary says about the term:

1. a piece of equipment or furniture that is fixed in position in a building or vehicle: a light fixture.
   • (fixtures) articles attached to a house or land and considered legally part of it so that they normally remain in place when an owner moves: the hotel retains many original fixtures and fittings. Compare with fitting ( sense 1 of the noun)).
   • informal a person or thing that is established in a particular place or situation: palm readers were a fixture in most '40s nightclubs.
2. British a sports event that takes place on a particular date.

None of the above sounds like a good description of test data to me.

[noniq]

I think “test fixtures” is a fairly common term at least in the Rails world:

What Are Fixtures? Fixtures is a fancy word for sample data.

(Source: http://guides.rubyonrails.org/testing.html#the-low-down-on-fixtures)

The guide talks about test data in the database only, but Rails itself keeps a lot of file based test data (eg. view templates) in the fixtures directory, too. See for example https://github.com/rails/rails/tree/master/actionpack/test/fixtures

[noniq]

So let’s try to get this shipped! 🚢

I reverted RVM project file detection back to the old method, and also added checking for a Ruby version in the Gemfile. Is there anything else left?

[jacob-carlborg]

I'll give it another look.

[noniq]

Okay, so one more thought about RVM (caused by https://github.com/textmate/ruby.tmbundle/pull/104#discussion_r81365193):

Currently, we use RVM only if we find a RVM project file. This has two problems:

• It is rather complicated (RVM supports many different project files, as well as simply specifying a ruby version in Gemfile:
• It won’t work (I suppose) if you have a default Ruby version set via RVM (and no project file in the current project).

So what about simply removing project file detection and using RVM always if it installed? (That would also mirror the behaviour for rbenv: If it is installed, use it).

[jacob-carlborg]

So what about simply removing project file detection and using RVM always if it installed? (That would also mirror the behaviour for rbenv: If it is installed, use it).

How would that solve your second point?

[noniq]

Right … I initially thought of simply always prefixing commands with rvm . do – but as you already mentioned, this won’t work in the case a default Ruby is set. Okay, so for detecting an executable in path we also have to check if RVM is installed and a project file is present, and if so, use rvm . do which …. Will update the code accordingly.

[noniq]

Okay, should be fixed now with 9e4b970!

[jacob-carlborg]

An alternative way to deal with this RVM stuff could be to just invoke rvm . do with a command that should always pass, something like rvm . do true. If that command fails it means that no project file exists.

BTW, I noticed that invoking a command using rvm . do adds significant overhead. Invoking true takes 0.05 seconds on my machine, will invoking the same command prefixed with rvm . do takes up to 0.498 seconds. That means that if someone writes a bundle command, using this function, without being careful (like running the command in the background), will stall TextMate for half a second.

[noniq]

That’s a nice idea, using rvm . do true! I’m not sure what to do about the overhead, though … any ideas?