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

Updated Windows compatibilty patches #979

Closed
wants to merge 11 commits into from

Conversation

alex85k
Copy link
Contributor

@alex85k alex85k commented Apr 7, 2014

I have updated Windows compatibility code from
#880
to simplify futher integration.
#646

Many problematic places (like asm code) were already fixed in develop branch, only "big" fixes that remains are UUID renaming, path.c_str() -> path.string().c_str() replacement and xsi shared memory alternative. Tests pass on Windows correctly (when cuke_datastore is not included).

@DennisOSRM DennisOSRM mentioned this pull request Apr 7, 2014
@DennisOSRM
Copy link
Collaborator

@alex85k thanks for the updated patch set. Much appreciated! Do you think we should rename the UUID(C) class to give it a better and more telling name?

@alex85k
Copy link
Contributor Author

alex85k commented Apr 7, 2014

I am not very good at naming classes, just tried to make the smallest possible change. :)
Something like OsrmBuildID could work but it is rather long.

@DennisOSRM
Copy link
Collaborator

Right, let me meditate a bit on class names. Now, I got to setup the Windows build slave for our Jenkins CI instance.

@alex85k
Copy link
Contributor Author

alex85k commented Apr 7, 2014

Oh, I forgot the updated instructions and batch files: https://gist.github.com/alex85k/8637217#file-a_readme-md
Just tested them on VS2012(need to patch stxxl) and VS2013 (need to patch Boost 1.55).

@DennisOSRM
Copy link
Collaborator

@alex85k quick question before I dive into it: Did all the tests pass on Windows?

@alex85k
Copy link
Contributor Author

alex85k commented Apr 7, 2014

Yes, the tests did pass correctly (checked before adding 0624f33 on VS2013 build). Will run recheck to be sure (will post results tomorrow).

@DennisOSRM
Copy link
Collaborator

@alex85k awesome. thanks

@alex85k
Copy link
Contributor Author

alex85k commented Apr 7, 2014

I forgot to add - 127.0.0.1 on newer Windows could be much faster than localhost (IPV6 setup problems).

The tests finished, 8 are failing :(

cucumber features\options\files.feature:19 # Scenario: Passing base file
cucumber features\options\help.feature:7 # Scenario: Help should be shown when no options are passed
cucumber features\options\help.feature:30 # Scenario: Help, short
cucumber features\options\help.feature:53 # Scenario: Help, long
cucumber features\options\invalid.feature:7 # Scenario: Non-existing option
cucumber features\options\invalid.feature:14 # Scenario: Missing file
cucumber features\options\version.feature:10 # Scenario: Version, short
cucumber features\options\version.feature:17 # Scenario: Version, long

I guess it is because special color-realted symbols are printed on console.

@alex85k
Copy link
Contributor Author

alex85k commented Apr 7, 2014

Another sample:

  Scenario: Version, long                                                # features\options\version.feature:17
    When I run "osrm-routed --version"                                   # features/step_definitions/options.rb:1
    Then stderr should be empty                                          # features/step_definitions/options.rb:49
      U+043D to US-ASCII in conversion from IBM866 to UTF-8 to US-ASCII (Encoding::UndefinedConversionError)
      ./features/step_definitions/options.rb:50:in `/^stderr should be empty$/'
      features\options\version.feature:19:in `Then stderr should be empty'
    And stdout should contain 1 line                                     # features/step_definitions/options.rb:53
    And stdout should contain /(v\d{1,2}\.\d{1,2}\.\d{1,2}|\w*-\d+-\w+)/ # features/step_definitions/options.rb:35
    And it should exit with code 0                                       # features/step_definitions/options.rb:23

@alex85k
Copy link
Contributor Author

alex85k commented Apr 7, 2014

When running manually, I get

R:\build\11\Project-OSRM\build>osrm-routed.exe  --version
[info] v0.3.7-290-g6f5deb0←[0m

Maybe ←[0m is the problem?

@DennisOSRM
Copy link
Collaborator

hm, ok. that's coming from SimpleLogger. I'll look into that. it shouldn't be too hard to fix.

DennisOSRM added a commit that referenced this pull request Apr 7, 2014
@DennisOSRM
Copy link
Collaborator

The commit above should help with the failing tests. @alex85k Can you run them again and report on the status?

@alex85k
Copy link
Contributor Author

alex85k commented Apr 8, 2014

Thank you, the tests are passing after using fixed develop as base brach and pathcing options.rb (adding .exe and quote marks).

273 scenarios (273 passed)
1076 steps (1076 passed)
10m22.433s

(I rebased to develop branch and did git push --force. Hope it will work on Github)

@alex85k
Copy link
Contributor Author

alex85k commented Apr 8, 2014

Gem install bundle failed on Travis. It is not me :)

@emiltin
Copy link
Contributor

emiltin commented Apr 8, 2014

looks like it's just a network problem on travis:
Unable to download data from https://rubygems.org/ - Errno::ETIMEDOUT: Connection timed out - connect(2)

@DennisOSRM
Copy link
Collaborator

Let me just restart the thing.

@DennisOSRM
Copy link
Collaborator

@alex85k passing now.

@alex85k
Copy link
Contributor Author

alex85k commented Apr 9, 2014

Good! If any further testing is needed, let me know.
(updated build instruction: https://gist.github.com/alex85k/8637217#file-a_readme-md )

@DennisOSRM
Copy link
Collaborator

Thanks. Will do

@rfrezino
Copy link

@alex85k very good! I'm really in troubles trying to compile OSRM for Windows. Do you have some "place" with the binaries ?

Tks

@alex85k
Copy link
Contributor Author

alex85k commented Apr 15, 2014

@rfrezino :
There are "unsupported" non-updating binaries I made for Russian forum:
https://dl.dropboxusercontent.com/u/63393258/OSRM-Windows-038.zip
Pre-windows-7-build(x64):
https://dl.dropboxusercontent.com/u/63393258/OSRM-WindowsXP-038.zip]OSRM-WindowsXP-038.zip

We are waiting for Windows build server and official builds)

@rfrezino
Copy link

@alex85k Ok, tks again. I'll keep following your project, it's going to be very useful.

@DennisOSRM
Copy link
Collaborator

So, here is the plan to get this into mainline:

  • merge other feature branch first
  • rebase (then) onto develop branch
  • set up Windows VM for CI Server
  • configure export of build artifacts
  • actually hit merge button

@DennisOSRM DennisOSRM mentioned this pull request Apr 16, 2014
@clns
Copy link

clns commented Apr 17, 2014

After spending a day trying to follow the Windows Compilation guide - with no success in the end - I stumbled upon this. So am I understanding correctly that the guide is outdated, and official builds are going to pop up very soon? I'd like to be able to debug the code.

@alex85k
Copy link
Contributor Author

alex85k commented Apr 17, 2014

@calinseciu Strange, I had updated the guide recently (see also discssion in
http://forum.openstreetmap.org/viewtopic.php?pid=411387#p411387 )
(Wiki guide is outdated)

https://gist.github.com/alex85k/8637217#file-a_readme-md
(it is for release version, however)

@clns
Copy link

clns commented Apr 17, 2014

@alex85k oh I was not aware of this guide. Although no debugging, it should be great to be able to finally compile the project. I'll give it one more try, thanks.

@alex85k
Copy link
Contributor Author

alex85k commented Apr 17, 2014

It can be compile it as debug too (just use debug version of Boost and replace CMAKE_BUILD_TYPE in scripts).

DennisOSRM added a commit that referenced this pull request Apr 21, 2014
@alex85k
Copy link
Contributor Author

alex85k commented Apr 29, 2014

Let me know when the rebased patch is needed :)

@DennisOSRM
Copy link
Collaborator

anytime ;)

@alex85k
Copy link
Contributor Author

alex85k commented Apr 30, 2014

Here it is. Pathches are even smaller now.
(by the way, I have no idea why ca11422 , 1b7a73d , ee7e81f are in this branch)

@DennisOSRM
Copy link
Collaborator

Great, can you rebase on develop?

@alex85k
Copy link
Contributor Author

alex85k commented Apr 30, 2014

I did rebase them to develop but pushed them to this pull-request for master. Should I create new PR?

One single test is failing on Windows: features/testbot/oneway.feature ( Tables were not identical (Cucumber::Ast::Table::Different) ). I have no idea why.

@DennisOSRM
Copy link
Collaborator

You should be able to just change the target branch.

@alex85k
Copy link
Contributor Author

alex85k commented Apr 30, 2014

@DennisOSRM
Copy link
Collaborator

Alright then lets close here.

@DennisOSRM DennisOSRM closed this Apr 30, 2014
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.

5 participants