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

v 0.8.0 'which' is unix specific (doesn't work on Windows) #284

Closed
weedySeaDragon opened this issue Jul 14, 2015 · 21 comments
Closed

v 0.8.0 'which' is unix specific (doesn't work on Windows) #284

weedySeaDragon opened this issue Jul 14, 2015 · 21 comments

Comments

@weedySeaDragon
Copy link
Contributor

'which' is a unix-specific command; it doesn't exist on Windows ('where' is a close equivalent). Also, checking a PATH for the existence of a command does not work the same in Windows.

There is code in spawn_process.rb and debug_process.rb that uses 'which' and checks the PATH. This won't work on Windows. I'm not clear on what the intent is. I can help with a PR if you can help me better understand the intent.

@maxmeyer
Copy link
Member

Somehow path resolution fails for jruby + childprocess. That's why I added which.

@weedySeaDragon
Copy link
Contributor Author

Ok, but now you've broken Windows. :-0
If that approach (checking the PATH) makes sense for the unix systems, then sounds like a conditional is needed so that you have a working alternative approach for windows. Since you'll need that conditional check in a few places (e.g. 'is this running on windows?') I suggest that you add a utility method somewhere for that check. ChildProcess has a good set of methods for checking the os. I'm not sure if you can use them; I don't know if you have a ChildProcess instantiated everywhere when you need to check the os. Or you can use the same approach that ChildProcess uses.
Or you can certainly make some child classes of SpawnProcess that each know how to build and run a process for their respective OS. (or OS-specific classes that are components of SpawnProcess, etc.) I don't know if that amount of work is worth it right now for you, and I don't know how much work that might or might not be. Just throwing it out there.

I can certainly help write or incorporate the methods for checking which os it is.

@ghost
Copy link

ghost commented Jul 15, 2015

BTW: I'm not using the which-utility but a helper method https://github.com/cucumber/aruba/blob/master/lib/aruba/platform.rb#L177. First of all I like to get that one fixed. If you can help with that?

@weedySeaDragon
Copy link
Contributor Author

Thanks for straightening me out on that, @dg-ratiodata . My bad for assuming it was the which utility. Since it's an aruba method, that should make it easier to address.
Yes, I can help.
The main issue is still: the strategy of check the PATH to see if a command exists in it doesn't work for Windows. On Windows, a command (or executable (e.g. .bat, .cmd, .exe, etc.) ) just needs to live (a) in a directory specified by ENV[PATH] or (b) in a directory that the Windows OS automatically already includes in it's search (like Program Files or system32 or many others. It can be nearly impossible to check all of the directories that Windows automatically already includes.
Using the Windows specific where command for Windows might be what you're after -- I'm not exactly sure of the goal of checking. Is it to catch a potential failure of trying to run a non-existent command before it happens with ChildProcess? I don't understand why you are taking responsibility of catching the error instead of letting ChildProcess raise it.
I think that putting methods into platform.rb that will return is this running on a Windows OS? is the first step. That can then be used in other places by aruba. Does that seem like a good idea? If so, I'll work on that first.
Then once I understand why you're doing the check with the PATH, we (I) can write the code that will do the Windows-specific checking in the which utility method (e.g. in https://github.com/cucumber/aruba/blob/master/lib/aruba/platform.rb#L177).

@maxmeyer
Copy link
Member

I don't understand why you are taking responsibility of catching the error instead of letting ChildProcess raise it.

On jruby childruby fails to find commands using PATH. I have no idea why. Using which was the first thing which came into my mind to fix it. At least on posix platforms it worked. ;-)

I think that putting methods into platform.rb that will return is this running on a Windows OS? is the first step.

Sounds reasonable. What about Platform.ruby_platform? :windows? Better suggestion?

Then once I understand why you're doing the check with the PATH, we (I) can write the code that will do the Windows-specific checking in the which utility method (e.g. in https://github.com/cucumber/aruba/blob/master/lib/aruba/platform.rb#L177).

Anyway. Maybe we should split the which method into better separated parts, like WhichPosix and WhichWindows. Given that we have a method to determine the platform we're running on we could then select which implementation to use. Something similar to SpawnProcess, DebugProcess.

How are commands like rake run on Windows? Normally there's some kernel magic involved on Posix systems, but there's no .bat or .exe to run other than ruby c:\path\to\rake.

BTW: On a Windows-system I've access to system32 is part of PATH. I think we should assume that the PATH is correct, as it is used for cmd.exe as well - at least from my experience.

@weedySeaDragon
Copy link
Contributor Author

On jruby childruby fails to find commands using PATH. I have no idea why. Using which was the first thing which came into my mind to fix it. At least on posix platforms it worked. ;-)

Since JRuby runs on multiple platforms, you'll need a platform-independent solution for it. (So the PATH approach might work some of the time, when someone happens to be running JRuby on a POSIX-compliant OS, but will fail other times.)

I still don't understand the problem you're trying to solve with the which method. When you say "On jruby childruby fails to find commands using PATH. " What functionality is aruba providing that is specific to a user's PATH? Why is the question that is not clear to me. (I've looked at the specs and features and can't figure it out from those, either.)
I wonder if you're trying to solve a general problem with a too-specific solution. IOW, is the problem just that ChildProcess fails on JRuby ? If that's really the problem, then have you narrowed it down to it just failing on Posix systems? Or is it always failing? (In which case there might be a problem with how ChildProcess is being used, perhaps analagous to the problem with Windows -- see #283 , or in part because perhaps the use of ChildProcess needs to vary depending on the os. Or maybe there are specific issues with ChildProcess and JRuby.) If you've already investigated that and the right solution is to work with the PATH on Posix systems, then OK. I just want to understand that before I can help with a solution on Windows. Understanding why you're working with PATH will let me know if that's necessary for a Windows os, and if so, how to accomplish it. (If the problem is limited to Posix, then we don't necessarily need to deal with the PATH at all under Windows.)
If the problem is more general, then perhaps I can suggest a solution that isn't so platform dependent. And if I can't, then we can investigate other gems and systems to see how they've solved the problem. Plenty of people have had to figure out how to make this stuff run across platforms for a number of years. We can take advantage of the hard work they've already done. :-)

Here's another way for me to ask the questions:
If you were to write up a cucumber feature for this, how would it start?
(We should write at least one rspec test for this anyway. And I'm guessing well need at least one test per os.)

Thanks for your patience with me on this.


How are commands like rake run on Windows? Normally there's some kernel magic involved on Posix systems, but there's no .bat or .exe to run other than ruby c:\path\to\rake.

On Windows, as gems or utilties are installed, generally a .bat file is also put into < path-to-ruby-version >\bin The .bat file generally calls the ruby interpreter with a ruby file to run the given 'command' (like rake) and any arguments. So the rake.bat file essentially is something like: "ruby.exe" "< path-to-ruby-version >/bin/rake" %1 %2 %3 ... (with some additional code to handle older WinNT systems, etc.).

@ghost
Copy link

ghost commented Jul 16, 2015

Wow. :-) Thanks for the long helpful post. To make this clear from my understanding.

Using PATH

I still don't understand the problem you're trying to solve with the which method. When you say "On
jruby childruby fails to find commands using PATH. " What functionality is aruba providing that is
specific to a user's PATH? Why is the question that is not clear to me. (I've looked at the specs and
features and can't figure it out from those, either.)

All systems including Windows an Posix make use of a PATH-environment variable... As I'm very interested in having a solution which works across all platforms I decided to use PATH.

Sorry for the German Screenshots, I've got only a German one running here, but just to run my virtual machine.

unbenannt

unbenannt

From my understanding even Windows looks up commands using PATH. So this should be save to rely on that.

The which method should "abstract" away how the path to a command is resolved.

Problem with ChildProcess

I had issues with jruby (all versions in travis.yml) + childprocess. It constantly failed. Using which was a try to find out why it failed. And it worked. To get the release out of the door I left it as part of the API. So from my understanding, there's a problem of finding commands with childprocess. Googling does not help either. Maybe the problem is around thread safetynes - as we use class instance variables we might be not threadsafe. But I found no option to disable threads for the jvm for testing.

Using another library to solve problems on windows

I'm not that sure, if this is the way to go. Besides the windows and jruby problem I like the API and implementation of childprocess. It seems to be well maintained.

I think we've got some more pressing matters to be solved, e.g. cleanup and complete the test suite. I want to have a test suite I can rely on when releasing a new gem version, not breaking someon else's build process. But nevertheless you're more than welcome to have a look into this. 😄

Support for different platforms

As long as there are tests + a well defined and well documented interface aka class which wraps platform specific code I have no troubles with adding such stuff to aruba - as long as it is not tooo specific. But maybe @mattwynne can give his opinion on the platform stuff?

@weedySeaDragon
Copy link
Contributor Author

Thanks for explaining so much. I really appreciate it. Now I understand a lot more about why you're using the which method. (That's one of the major challenges with OSS: new people jumping in have to be brought up to speed to understand enough.)

My old Jr-high school German + my long history with Windows means I had no trouble with your German screenshots. :-)

I totally understand the need to get things out the door. (Do. I. Ever.)

And I'm not advocating anything like a new library just for Windows. (Maybe I'm misinterpreting your use of 'another library.') I'm just trying to figure out if we need to do one thing for Windows and other for 'everything else' -- like a few lines of code/ a method call. (That's not unusual at all when trying support multiple OSs.) As it's currently written, the PATH isn't working for windows. (It fails at ENV['PATHEX'] isn't the right environment variable. It should just be PATH, as in ENV['PATH'] )

There has to be at least one change made for Windows to use ChildProcess: #283 and I'll aim for a polymorphic process spawner with that. Having that in place will then provide some flexibility: if one ruby engine (like Jruby) or an OS (like Windows) requires something specific to make ChildProcess work, the framework -- including tests and any documentation -- will be in place. (And it will be interesting to see what happens with JRuby on Windows!)

How about if I do the work on #283 and then, if this still isn't working for Windows, we can revisit it at that point?

@maxmeyer
Copy link
Member

mmh.. can you try PATHEXT <--- T

@maxmeyer
Copy link
Member

How about if I do the work on #283 and then, if this still isn't working for Windows, we can revisit it at that point?

Good idea!

@weedySeaDragon
Copy link
Contributor Author

@maxmeyer My mistake. I did run the tests as is (which use "ENV['PATHEXT'] ). I typed it wrong in my comment above. (I should have either copied and pasted or just pointed to the line in the file.)
And I do have PATHEXT defined:

>> ENV['PATHEXT']
=> ".COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC"

But all of the tests in Aruba::Processes::SpawnProcess ( in spec/spawn_process_spec.rb) fail with

ArgumentError: ENV['PATH'] cannot be empty
./lib/aruba/platform.rb:185:in `which'
./lib/aruba/processes/basic_process.rb:46:in `which'
./lib/aruba/processes/spawn_process.rb:47:in `run!'
./spec/aruba/spawn_process_spec.rb:40:in `block (3 levels) in <top (required)>'
./lib/aruba/rspec.rb:44:in `block (2 levels) in <top (required)>'

The tests for run! fail at the same place, but with a little additional info about their matcher not matching:

expected Aruba::LaunchError, got #<ArgumentError: ENV['PATH'] cannot be empty> with backtrace:
  # ./lib/aruba/platform.rb:185:in `which'
  # ./lib/aruba/processes/basic_process.rb:46:in `which'
  # ./lib/aruba/processes/spawn_process.rb:47:in `run!'
  # ./spec/aruba/spawn_process_spec.rb:60:in `block (5 levels) in <top (required)>'
  # ./spec/aruba/spawn_process_spec.rb:60:in `block (4 levels) in <top (required)>'
  # ./lib/aruba/rspec.rb:44:in `block (2 levels) in <top (required)>'
./spec/aruba/spawn_process_spec.rb:60:in `block (4 levels) in <top (required)>'
./lib/aruba/rspec.rb:44:in `block (2 levels) in <top (required)>'

expected no Exception, got #<ArgumentError: ENV['PATH'] cannot be empty> with backtrace:
  # ./lib/aruba/platform.rb:185:in `which'
  # ./lib/aruba/processes/basic_process.rb:46:in `which'
  # ./lib/aruba/processes/spawn_process.rb:47:in `run!'
  # ./spec/aruba/spawn_process_spec.rb:54:in `block (5 levels) in <top (required)>'
  # ./spec/aruba/spawn_process_spec.rb:54:in `block (4 levels) in <top (required)>'
  # ./lib/aruba/rspec.rb:44:in `block (2 levels) in <top (required)>'
./spec/aruba/spawn_process_spec.rb:54:in `block (4 levels) in <top (required)>'
./lib/aruba/rspec.rb:44:in `block (2 levels) in <top (required)>'

I'm just skimming through the code, but I wonder if the issue is on this line:

 def run!
        # rubocop:disable  Metrics/LineLength
        fail LaunchError, %(Command "#{command}" not found in PATH-variable "#{environment['PATH']}".) unless which(command, environment['PATH'])

Could it be that somehow environment['PATH'] is an empty string for Windows? What other way could that parameter be empty when which is called?

@weedySeaDragon
Copy link
Contributor Author

and BTW, my path isn't empty. Here's my setup for working on aruba stuff right now, FYI:

ENV['PATH']
=> "C:\\rubys\\ruby-2.1.6-p336-x64\\bin;C:\\rubys\\ruby-2.1.6-p336-x64\\lib\\ruby\\gems\\2.1.0\\bin;C:\\rubys\\ruby-2.1.5-p273-x64-mingw32\\lib\\ruby\\gems\\2.1.0\\bin;.;.\\;C:\\rubys\\devkit-mingw64-64-4.7.2;C:\\rubys\\devkit-mingw64-64-4.7.2\\mingw\\bin;C:\\rubys\\devkit-mingw64-64-4.7.2\\bin;C:\\dev\\bin64\\OpenSSL-Win64\\bin;C:\\Windows\\SYSTEM32;C:\\Windows;C:\\bin;C:\\Program Files\\Java\\jdk1.7.0_25\\jre\\bin;C:\\Windows\\SYSTEM32\\WBEM;C:\\Program Files (x86)\\GnuWin32\\bin;C:\\Program Files (x86)\\GnuWin32\\contrib;C:\\Program Files (x86)\\GNU\\GnuPG\\pub;C:\\dev\\libraries\\win32;C:\\dev\\bin;C:\\ProgramData\\Oracle\\Java\\javapath;c:\\python27;c:\\python27\\Tools\\Scripts;C:\\Program Files\\ImageMagick-6.8.9-Q16;.... [snip]
"

@ghost
Copy link

ghost commented Jul 16, 2015

I'm just skimming through the code, but I wonder if the issue is on this line:

I think the error makes sense if ENV['PATH'] is really empty.

Not sure why it looks like this. Can you try to code walk using binding.pry? Starting with

before(:each) do
binding.pry
end

at the very beginning of the spec? ./spec/aruba/spawn_process_spec.rb. Plus walk through SpawnProcess from initalize.

@ghost
Copy link

ghost commented Jul 16, 2015

BTW I pushed some changes to master which might be helpful.

@mattwynne
Copy link
Member

Don’t forget you can natter on https://gitter.im/cucumber/aruba https://gitter.im/cucumber/aruba!

@weedySeaDragon
Copy link
Contributor Author

thanks for the reminder, @mattwynne ! heading there now..

@piotrmurach
Copy link

Sorry to awaken this old dragon, but I was thinking that I may have some value proposition. I'm developing suite of micro tools for developing CLI apps under umbrella of tty. And as so happens I've written tty-which for handling Unix which command across platforms. The library can be used independently and has no dependencies. I'm keen on providing a single reusable solution rather than going over reinventing the wheel. But I understand that you may not want to have another dependency. Any thoughts?

@ghost
Copy link

ghost commented Jan 19, 2016

But I understand that you may not want to have another dependency.

Personally, I prefer to keep the dependency list as short as possible. But if you like I'm more than happy to accept a PR fixing our "windows" problem(s).

@stale
Copy link

stale bot commented Nov 9, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

@stale stale bot added the stale These issues were closed by stalebot and need to be reviewed to see if they're still relevant. label Nov 9, 2017
@stale
Copy link

stale bot commented Nov 16, 2017

This issue has been automatically closed because of inactivity. You can support the Cucumber core team on opencollective.

@stale stale bot closed this as completed Nov 16, 2017
@mvz
Copy link
Contributor

mvz commented Jan 4, 2020

There is now separate which code for windows so this should work properly.

@mvz mvz removed the stale These issues were closed by stalebot and need to be reviewed to see if they're still relevant. label Jan 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants