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

provide methods to query which OS is running. Use FFI::Platform methods #290

Conversation

weedySeaDragon
Copy link
Contributor

I've added some methods into (lib)/api/platform.rb to query which OS is running. Since FFI is already being included by the project and FFI already has excellent methods for querying the OS, the methods are essentially pass-thru's to what FFI does.

If it's somehow not good to use ffi (it did require a require 'ffi' in platform.rb) I can use the already required rbconfig. (Tho I'll likely be re-writing what FFI has done.)

@weedySeaDragon
Copy link
Contributor Author

I've now added RSpec methods so that we can test -- and change -- the current operating system, ruby_platform, and ruby_engine. The current os is gotten from FFI::Platform; these test mock FFI::Platform with a class_double. These spec also use stub_constant for RUBY_ENGINE and RUBY_PLATFORM -- so those can be changed, too.
The intent is that these tests are the basis for being able to instrument those values (the current OS, RUBY_PLATFORM, and RUBY_ENGINE) as needed so we can test the system behavior without always having to run a travis or appveyor job. This should make it easier to develop tests for behavior based on the state of one of those. (Like #283, #284, #285)
The tests are in a file named spec/aruba/platform/os_platforms_engines_spec.rb
-- which I think is a poor file name. It's descriptive, but seems long. Should the file name be changed? I want to be sure to keep with the standards and feel of this project.
What do you think of these, @dg-ratiodata (and @maxmeyer )? I want to get these right so that I can build on them for the other windows-related PRs that I'm working on.

@mattwynne
Copy link
Member

FYI @weedySeaDragon, @dg-ratiodata and @maxmeyer are the same person. It confuses me too 😄

@weedySeaDragon
Copy link
Contributor Author

@mattwynne ya, I learned that the other night on Gitter. I referenced both of those as kind of a joke about exactly that. :-)

@mattwynne
Copy link
Member

Maybe I'm missing something but I don't see why we need the specs in os_platforms_engines_spec.rb at all. Are they designed to be used elsewhere, or are they just to test the new methods added to Aruba::Platform in this PR?

@weedySeaDragon
Copy link
Contributor Author

Yes and yes. These do test the new methods in Aruba::Platform, but then can be expanded (perhaps as shared examples or contexts) to also be used for the PRs that require different behavior based on the current OS.
We'll need a way to test the behaviors that are different on Windows, like the file permissions (#285), the different spawned process (#283), etc. In order to do that kind of testing, we first need to instrument the current test configuration (context, literally) so that the answer to "on we running on windows?" is controlled. These specs provide that. I want to make sure these are OK before I go much farther with these.
If using these as the basis for some of the testing that needs to be done with the other os-specific PRs/issues, is not the way to go, then now's the time for me to understand that. Make sense?

@mattwynne
Copy link
Member

Rather than spreading the conditional logic about the OS around the code, I'd prefer to isolate it at the edges. Let's build some different instances of the Platform duck-type for the different platforms, and then we can keep all the OS-specific code in one place.

I'll see if I can knock up a PR to show you what I mean.

@weedySeaDragon
Copy link
Contributor Author

Sounds good. closing this.

@maxmeyer
Copy link
Member

@mattwynne I started breaking up Platform and even Api#methods into smaller classes. But only if needed or it makes sense. Just for you to know.

@weedySeaDragon
Copy link
Contributor Author

Should've just reverted the commits (not closed this whole thing)

(max - sounds like you and should get coordinated about what should be done on Platform and SpawnProcess so that we don't duplicate work.)

@maxmeyer
Copy link
Member

(max - sounds like you and should get coordinated about what should be done on Platform and SpawnProcess so that we don't duplicate work.)

Yes. Personally I like to see what @mattwynne has in mind with I'd prefer to isolate it at the edges..

From my point of view, having a class for edge cases (WindowsEnvironment, PosixEnvironment, WindowsWhich, PosixWhich) would be the way to go as this approach can handle both different data structures and different algorithms.

I don't want to bloat Platform too much as it includes way to many methods already.

@mattwynne
Copy link
Member

When I say "edges" I mean the edge of the architecture - use a ports & adapters design so we can pass in an adapter for the platform-specific stuff, and all method calls that need platform-specific implementations go across a common port. I think all of us are on the same page now (@weedySeaDragon and I already caught up about this on gitter)

@ghost
Copy link

ghost commented Jul 22, 2015

See #299 for a possible implementation for "a ports & adapters design". I use ffi directly here.

@ghost ghost force-pushed the master branch 2 times, most recently from 4b424bf to bbd9993 Compare July 22, 2015 10:50
@ghost
Copy link

ghost commented Jul 23, 2015

As we've got the check in Platform now it does not make sense to add those methods to Platform. Sorry @weedySeaDragon for the time you spent on this. I used your feedback and changed the Unix.match?-method a bit.

@ghost ghost closed this Jul 23, 2015
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants