Skip to content
This repository has been archived by the owner on Jun 25, 2023. It is now read-only.

Issue #16 - Getting Landrush to work on Windows #154

Closed
wants to merge 9 commits into from
Closed

Issue #16 - Getting Landrush to work on Windows #154

wants to merge 9 commits into from

Conversation

hferentschik
Copy link
Contributor

This is a first stab at resolving #16 and getting Landrush to work on all OSes. The key changes are:

Apart from that the changes of this pull request are consequences of the two points mentioned above.

In command.rb I also removed the use of /usr/bin/pr (which does not exist on Windows) with printf. Last but not least, I added some documentation for configuring DNS on Windows.

I tested against Windows 10 and it works. I still need to test on Windows 7. There is no issue with running the Landrush DNS server, but rather a question of differences in the DNS configuration. On Windows 7 it might for example be necessary to configure the "DNS suffix list" for the adapter to which one adds the Landrush DNS server.

If we agree to merge this pull request, the next steps would be:

  • Automate Windows DNS configuration
  • Make tests runnable on Windows
  • Get greater test coverage
    These things could be addressed in follow up issues.

Looking forward to get some feedback on this.

@hferentschik hferentschik changed the title Issue #16 Getting Landrush to work on Windows Issue #16 - Getting Landrush to work on Windows Jan 15, 2016
@hferentschik hferentschik mentioned this pull request Jan 15, 2016
Closed
@maxandersen
Copy link

Looks like the gemfile.lock picks up a newer RuboCop version that is not fully backwards compatible.

Seems adding something like what is outlined in this PR https://github.com/Homebrew/homebrew/pull/48144/files could work.

@hferentschik
Copy link
Contributor Author

Looks like the gemfile.lock picks up a newer RuboCop version that is not fully backwards compatible.

TBH I did not run Rubocop. I think it was added just a while ago and I just rebased on top of these changes. Personally I only ever ran rake with explicit target which did not show any problems. However, looking at the Rakefile the rubocop task is part of the default task.

You can run rake build or rake test directly to bypass this. I see what I can do regarding rubocop itself...

@hferentschik
Copy link
Contributor Author

I fixed some basic spacing issues, but other than that just re-generated .rubocop_todo.yml, since it looked like that most errors were just ignored anyways. Not sure whether all default Rubocop checks make sense anyways. Happy to discuss style once we have an overall agreement on this pull request.

@hferentschik
Copy link
Contributor Author

I am wondering what environment the CI job is running on. Linux? It reports:

Finished tests in 96.890238s, 0.4335 tests/s, 0.6193 assertions/s.

  1) Failure:

test_0004_stops the landrush server when there are no dependent machines left(    Landrush::Action::Teardown) [/home/travis/build/phinze/landrush/test/landrush/action    /teardown_test.rb:48]:

Expected: false

  Actual: true

  2) Failure:

test_0001_starts and stops a daemon(Landrush::Server::start/stop) [/home/travis/    build/phinze/landrush/test/landrush/server_test.rb:23]:

Expected: false

  Actual: true

42 tests, 60 assertions, 2 failures, 0 errors, 2 skips

rake aborted!

My result of rake test on OS X is:

Finished tests in 37.004713s, 1.1350 tests/s, 1.7025 assertions/s.

42 tests, 63 assertions, 0 failures, 0 errors, 1 skips

I can try to run the tests in a Linux VM, but it would be nice to know the environment used for CI to make sure I align with it.

@hferentschik hferentschik force-pushed the issue-16-spawn branch 2 times, most recently from 18351da to 674a69f Compare January 21, 2016 11:18
@hferentschik
Copy link
Contributor Author

Ok, I pushed another update to this pull request, fixing the tests on Linux as well. Turns out that I missed a Process.detach after spawning the Landrush DNS server. Seems OS X and Windows are more forgiving, where on Linux you end up with Zombie process which won't go away until the parent process exits.

@hferentschik
Copy link
Contributor Author

@strzibny - WDYT?

@hferentschik hferentschik mentioned this pull request Jan 27, 2016
Closed
@hferentschik
Copy link
Contributor Author

Rebased on current master.

@LalatenduMohanty
Copy link

@hferentschik bundle exec rake works fine on Fedora 23. However I had to add gem 'json' to Gemfile to make it work

[lmohanty@LalatenduM-laptop landrush-hardy]$ bundle exec rake
Running RuboCop...
Inspecting 44 files
............................................

44 files inspected, no offenses detected
Run options: --seed 7723

# Running:

...Stopping daemon...
Pid file /tmp/vagrant_landrush_test_working_dir/run/landrush.pid not found. Is the daemon running?
.Stopping daemon...
Pid file /tmp/vagrant_landrush_test_working_dir/run/landrush.pid not found. Is the daemon running?
.Stopping daemon...
...Stopping daemon...
Pid file /tmp/vagrant_landrush_test_working_dir/run/landrush.pid not found. Is the daemon running?
.Stopping daemon...
.Stopping daemon...
Pid file /tmp/vagrant_landrush_test_working_dir/run/landrush.pid not found. Is the daemon running?
.Stopping daemon...
.Stopping daemon...
..Stopping daemon...
.Stopping daemon...
.....Stopping daemon...
........S......Stopping daemon...
.Stopping daemon...
.Stopping daemon...
.SStopping daemon...
...

Finished in 16.901247s, 2.4850 runs/s, 3.5500 assertions/s.

42 runs, 60 assertions, 0 failures, 0 errors, 2 skips

You have skipped tests. Run with --verbose for details.

@hferentschik
Copy link
Contributor Author

However I had to add gem 'json' to Gemfile to make it work

Hmm, interesting. I thought JSON is part of the standard lib, so it should not be necessary to install an additional gem. What Ruby version are you using?

@LalatenduMohanty
Copy link

@hferentschik

$ ruby --version
ruby 2.2.4p230 (2015-12-16 revision 53155) [x86_64-linux]

@hferentschik
Copy link
Contributor Author

Interesting. I guess it cannot harm to explicitly add 'json'. trying to understand why is it required. Did you specify any particular version?

@LalatenduMohanty
Copy link

@hferentschik nope, it was just gem install json then added to Gemspec

@LalatenduMohanty
Copy link

@hferentschik I think you need to update https://github.com/phinze/landrush/blob/master/CHANGELOG.md in this PR to list the fix

@hferentschik
Copy link
Contributor Author

Sure. The change log is not updated, nor the version bumped. This would
happen
when a new release is cut. Probably making a bit of a version jump as
well!?
Thanks for the feedback btw. Much appreciated.

@hferentschik
Copy link
Contributor Author

@LalatenduMohanty I've added the json gem to the Gemfile and pushed an update. Can you try once more?

@strzibny
Copy link
Contributor

strzibny commented Feb 6, 2016

You need json for Ruby packaged in Fedora, it's unbundled from Ruby itself.
On Feb 5, 2016 4:54 AM, "Lalatendu Mohanty" notifications@github.com
wrote:

@hferentschik https://github.com/hferentschik nope, it was just gem
install json then added to Gemspec

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

@hferentschik
Copy link
Contributor Author

You need json for Ruby packaged in Fedora, it's unbundled from Ruby itself.

Go figure.

@LalatenduMohanty
Copy link

@hferentschik now bundle exec rake works fine in Fedora 23 without any modification

@hferentschik
Copy link
Contributor Author

now bundle exec rake works fine in Fedora 23 without any modification

nice

@hferentschik
Copy link
Contributor Author

maybe just use the autocorrect feature to remove the overall amount of errors

+1

Sorry for giving style comments first, I am just not able to give the PR a test drive for the next days

Sure. No problem. Just happy to get some feedback. I also found some other related issues which I will address in follow up pull requests. For example the way the Vagrant data dir is determined is wrong. One need to go via Vagrant @env instance which is passed to the plugin. This does not really effect this code, but can lead to problems on Windows where you want to start Landrush from a cygwin shell.

I just like to have something, and improving from there.

+1 Sounds like a good plan.

strzibny
strzibny reviewed Feb 19, 2016
View reviewed changes
lib/landrush/server.rb
ensure_path_exits(log_file)

if OS.windows?
pid = spawn('ruby', "#{__FILE__}", "#{port}", "#{working_dir}", :chdir => working_dir.to_path, [:out, :err] => [log_file, "w"], :new_pgroup => true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the new Hash syntax here?

@strzibny
Copy link
Contributor

I included few styles issues that I would like to change. As for real Windows testing, I don't have the Windows machine for testing that I had. @pvalena can you please test this patch?

@pvalena
Copy link

pvalena commented Feb 19, 2016

@strzibny Hi, sorry i am really busy right now, but as soon as I get it done I will test it.

@hferentschik
Copy link
Contributor Author

@strzibny, thanks for the feedback. I am planning another update to this pull request for next week. I'll address your style comment and try to reduce the overall amount of robocup violations.

@hferentschik hferentschik mentioned this pull request Feb 22, 2016
Closed
@hferentschik
Copy link
Contributor Author

@fh, @strzibny - I did a fair bit of code cleanup and reduced the amount of rubocop violations considerably. Forced pushed an update to this pull request.

Also created a follow up pull request #160 based on this pull request addressing issue #157.

@bexelbie How is the review going?

@hferentschik
Copy link
Contributor Author

Here is another idea. The problem seems to be access for people to Windows machines. What's about marking Windows support as experimental? If we can agree that the code is not breaking anything for Linux or OS X, we could then go ahead and merge. We could even do a release and this way get feedback from people trying to use Landrush on Windows.

@bexelbie
Copy link
Contributor

@hferentschik moving to windows as experimental could work - I am traveling this week so my windows testing time is more limited :(

@strzibny
Copy link
Contributor

strzibny commented Mar 5, 2016

@hferentschik like that idea very much.

@hferentschik
Copy link
Contributor Author

like that idea very much.

Ok, let's go for something like that then. I guess all that's needed are some notes in the docs

@bexelbie
Copy link
Contributor

bexelbie commented Apr 5, 2016

@hferentschik do you have time to rebase and add the docs notes? I saw your comment about wanting to land this, this week.

@hferentschik
Copy link
Contributor Author

do you have time to rebase and add the docs notes?

will do. I also will add a commit which allows to run the tests on Windows. Now you can develop Landrush on Windows as well :-)

I also confirmed the setup once more on a "virgin" Windows 10 setup. Worked fine.

I saw your comment about wanting to land this, this week.

Most likely tomorrow. Once everything is merged we will need to talk about a release :-)

@hferentschik
Copy link
Contributor Author

Ok, I pushed one more update adding a note to the docs that Windows support is experimental for now. I also changed the test suite and it runs now on Windows as well. So Landrush can now be run and developed on Windows.

I would like to merge this now. Can I get one more maintainer ACK - @njam, @ahpook, @strzibny, @fh, @bexelbie, @phinze?

hferentschik and others added 6 commits April 8, 2016 14:25
@hferentschik
Issue vagrant-landrush#16 Making Landrush work on Windows
9027b95 
- Replacing use of Rexec with plain Process#spawn calls
- Downgrading rubydns gem to 0.8.5 to use EventMachine instead of Celluloid IO which won't run on Windows
- Updating command.rb to no rely on Rexec
- Using printf in favor of /usr/bin/pr in order to format output in command.rb
- Adding os.rb to allow for OS check
@hferentschik
Issue vagrant-landrush#16 Documenting Windows DNS configuration
9dba4b1
@hferentschik
Issue vagrant-landrush#16 Adding Gemfile.lock to repository
755545f
@hferentschik
Issue vagrant-landrush#16 Updating .rubocop_todo.yml due to rule name…
8250c9f 
… chages in latest version and applying simple style fixes
@hferentschik
Issue vagrant-landrush#16 Making tests run on Windows
73458e4
@hferentschik
Issue vagrant-landrush#16 Adding Hardy Ferentschik to maintainer list
0806e7b
@ahpook
Copy link

ahpook commented Apr 9, 2016

@hferentschik looks like there's merge conflicts, according to github - do you need to rebase? I'm 👍 once that's resolved, I read through the docs and all seems reasonable.

@hferentschik
Copy link
Contributor Author

looks like there's merge conflicts, according to github - do you need to rebase?

I don't think so. I might have pushed one time too many to this branch. I'll can close this pull request and open a new one. That should sort things out.

I'm 👍 once that's resolved, I read through the docs and all seems reasonable.

Thanks.

@hferentschik
Copy link
Contributor Author

Actually I needed one more rebase. Nevertheless, here is the new pull request, passing also Travis CI - #170

@ezekg ezekg mentioned this pull request Apr 22, 2016
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@hferentschik @maxandersen @LalatenduMohanty @strzibny @bexelbie @fh @pvalena @ahpook @njam