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

Windows compatibilty patches (rebased to develop) #998

Merged
merged 10 commits into from
Jun 11, 2014

Conversation

alex85k
Copy link
Contributor

@alex85k alex85k commented Apr 30, 2014

copy of #979
with develop as base branch.

@alex85k
Copy link
Contributor Author

alex85k commented May 3, 2014

Debug version of the windows build is very strict-checking. It has "Checked Iterators" feature and give assertion error for certain things:

I tried to eliminate avoid these assertions, but the solution is to be checked.
Will commit the additional patch here when the testing finish.

@alex85k
Copy link
Contributor Author

alex85k commented May 3, 2014

@DennisOSRM There is only one failing test on Windows: features/testbot/oneway.feature .
I hope you'll be able to fix it. (does not depend on Debug/Release or compiler version)

Compilation was performed using free Visual Studio 2013 Express command prompt without any SDKs, batch files are from https://gist.github.com/alex85k/8637217 (had to replace one incompatible boost header too).

@alex85k
Copy link
Contributor Author

alex85k commented May 6, 2014

I tried to rebase patches to the latest develop version.

Using C++ 11 code automatically drops support for Visual Studio 2012 and older (even array initializer does not compile).
Should I remove the code from patch that is needed for older Microsoft Compiler versions too?

@DennisOSRM
Copy link
Collaborator

Yes, let's do this. We are moving towards C+11 now.

@alex85k
Copy link
Contributor Author

alex85k commented May 6, 2014

Did you invent a good name for UUIDC class? I can include it to updated patches.

@alex85k
Copy link
Contributor Author

alex85k commented May 7, 2014

I am updating the patch periodically to check compatibility with MSVC compiler.
I had to path all time-measuring code: alex85k@9d567af
(partial C++11 support or non-clear place in standard : http://en.cppreference.com/w/cpp/chrono/steady_clock/now , std::chrono::steady_clock::time_point is an actual class in MSVC)

@alex85k
Copy link
Contributor Author

alex85k commented May 7, 2014

Testing on Windows was successful (except features/testbot/oneway.feature ).

@DennisOSRM
Copy link
Collaborator

@alex85k thanks. Can you explain a bit what the issue on Windows is?

@alex85k
Copy link
Contributor Author

alex85k commented May 7, 2014

Testing issue is the one incorrect routing result (route not found):
https://gist.github.com/alex85k/994bffe1cd238003e300
It was already failing on older compilers (with unpatched Boost) and before the iterator-related commit
alex85k@334f1f0

@alex85k
Copy link
Contributor Author

alex85k commented May 7, 2014

Note: this test was passing on 2014-04-08 last time ).

@alex85k
Copy link
Contributor Author

alex85k commented May 7, 2014

I guess someone used git push --force on develop branch. Rebasing patch can be tricky... Let me know when it is really needed :)

@DennisOSRM
Copy link
Collaborator

yeah, sorry about pushing with --force. This shouldn't change anything to the actual code in develop.

@alex85k
Copy link
Contributor Author

alex85k commented May 7, 2014

Hear is the updated version.
MSVC does not support constexpr for now and does not have POSIX unistd.h. Could you please avoid them in future commits?

@DennisOSRM
Copy link
Collaborator

constexpr is becoming more and more important in many ways. It is a bummer that MSVC does not support this

@DennisOSRM
Copy link
Collaborator

Support for constexpr is in Nov 2013 CTP: http://www.microsoft.com/en-us/download/details.aspx?id=41151

@frankvandenbergh
Copy link

Hi Alex, Dennis,
Can/May I compile the new version 3.10 in windows?
Currently I use Alex' patches on 3.8.

@alex85k
Copy link
Contributor Author

alex85k commented May 14, 2014

Sorry, I did not manage to fix all constexpr and time_point differences today, will update the patch near the weekend.

@frankvandenbergh
Copy link

No problem Alex, I was just curious about the update (as the memory consumption of the 3.8 version is quite big).

@alex85k
Copy link
Contributor Author

alex85k commented May 16, 2014

Even after avoiding constexpr and tme_point difficulties, I have a compile error on MSVS 2013. It is caused by the code

            std::packaged_task<void()> server_task(std::bind(&Server::Run, routing_server));
            auto future = server_task.get_future();
            std::thread server_thread(std::move(server_task));

the problem seems to be described here (int() works but void() does not)
http://stackoverflow.com/questions/14744588/why-is-stdpackaged-taskvoid-not-valid
but I can not make a necessary fix or workaround myself. Can anyone help?

P.S. I am making some progress using lambda returning fake int instead std::bind... :)

@alex85k
Copy link
Contributor Author

alex85k commented May 16, 2014

Some workaround is implemented... I do not think that using CTP compiler is stable enough for end users, so maybe we can keep #ifdefs related to constexpr (that is not working on usual VS2013)?

@alex85k
Copy link
Contributor Author

alex85k commented May 16, 2014

@frankvandenbergh : you can use the published scripts with current version (MSVS 2013 Express - tested), win-038 is just a name, latest develop branch is used.

@frankvandenbergh
Copy link

Ok @alex85k I will try it.
In the build_osrm.bat should I use:
git clone https://github.com/alex85k/Project-OSRM.git Project-OSRM
or
git clone https://github.com/DennisOSRM/Project-OSRM Project-OSRM

@alex85k
Copy link
Contributor Author

alex85k commented May 19, 2014

@frankvandenbergh : For now, git clone https://github.com/alex85k/Project-OSRM.git Project-OSRM

@DennisOSRM : Is there any progress with Windows build server? ;)

@frankvandenbergh
Copy link

Hi Alex,
I constructed a VS2013 project using:
cmake .. -G "Visual Studio 12 Win64" -DCMAKE_BUILD_TYPE=Release -DBZIP2_INCLUDE_DIR=%PREFIX%/include -DBZIP2_LIBRARIES=%PREFIX%/lib/libbz2.lib -DCMAKE_INSTALL_PREFIX=%PREFIX% -DBOOST_ROOT=%BOOST_ROOT% -DBoost_USE_STATIC_LIBS=ON

Next, I build the project. 201 warnings, no errors, all executables were created.

The Extractor couldn't read the extractor.ini any more (fatal error). For me it isn't important as I only set the number of threads in there, and they are still 1. So I renamed the file.

It will try to extract a 2.6 Gb pbf file, after about one hour the CPU drops to zero and stays there (using 4.2 Gb memory as shown in the task manager).

This is the same procedure I did with 0.38 version (I only replaced the executables and cleaned the data dir). But then it ran smoothly.

The new osrm-routed.exe can read the extraction of the v0.38 version (I get the version warning). It returns only "cannot find routes".

@alex85k alex85k mentioned this pull request May 21, 2014
@alex85k
Copy link
Contributor Author

alex85k commented May 21, 2014

@frankvandenbergh:
Old files (generated before remove_geometry was included) are certainly not compatible with new router.

I'll check about the extractor.ini error.
2.6 Gb pbf should also be checked... If the file is a published extract, please give us a link. Anyway, this version certainly contains some bugs (1 simple routing test is failing) and is not production-ready. Maybe the hanging is related.

If you have the old sources in some folder (real 0.3.8), could you please archvie the Project-OSRM folder (without build) and put it here? I have lost the last working version when rebasing the patches and can not re-publish it.

@frankvandenbergh
Copy link

Hi Alex,

The pbf file is an extract of the latest europe file, generated using:
osmosis --read-pbf file=/E:/Frank/osm/europe-latest.osm.pbf outPipe.0=1 --bounding-box left=0.8349609 right=12.832031299999999 top=54.033586299999996 bottom=48.3416462 inPipe.0=1 outPipe.0=2 --write-pbf file=/E:/Frank/osm/europe-smaller.osm.pbf inPipe.0=2 omitmetadata=true

Here is the source code (don't know how to attach it): http://we.tl/YIvZvHQWN3

@alex85k
Copy link
Contributor Author

alex85k commented May 23, 2014

I can update the patches today (evening). Should we wait for some other branches and important changes? Is there some build server or testing Windows VM deployed?

@DennisOSRM
Copy link
Collaborator

Is there some build server or testing Windows VM deployed?

Not yet. We still have a licensing issue for Windows.

@alex85k
Copy link
Contributor Author

alex85k commented May 23, 2014

@DennisOSRM : So the license you got in January is not suitable? Maybe Mapbox have some existing servers or licenses for other projects?

Reworking the patch I got the following compile error:
F:\build\Project-OSRM\prepare.cpp(313) : error C2338: changing ImportEdge type has influence on memory consumption!

It is triggered because EdgeBasedEdge does not have size 16. Really, It have 4 integers and 2 booleans inside... I do not know how to solve the problem, just commented this assertion temporarily (may lead to errors?).

@alex85k
Copy link
Contributor Author

alex85k commented Jun 11, 2014

OK. It is also really slow. But the compilation incompatibilities in future are unlikely (it is almost gcc except does not have some unix libraries). The necessary fix included one more failing sizeof static assertion, disabling cpuid include and explicitly adding wsock and gomp libraries.

@alex85k
Copy link
Contributor Author

alex85k commented Jun 11, 2014

Rebased this once more, changed one patch alex85k@be5c3e4 to be mingw-compatible (other _MSC_VER and WIN32 were placed correctly).

@alex85k
Copy link
Contributor Author

alex85k commented Jun 11, 2014

Any more changes to be done? Is this planned for merging after #1069 or something else will arrive?

@DennisOSRM
Copy link
Collaborator

Thanks @alex85k. I'll get to it this afternoon

@DennisOSRM DennisOSRM merged commit f1bde40 into Project-OSRM:develop Jun 11, 2014
@alex85k
Copy link
Contributor Author

alex85k commented Jun 11, 2014

Wow! It happened! Even with reformatting and FTP!
Thank you very much :)

@DennisOSRM
Copy link
Collaborator

Thank you. Great work.

@DennisOSRM
Copy link
Collaborator

Going to announce it on the mailing list shortly.

@emiltin
Copy link
Contributor

emiltin commented Jun 11, 2014

well done guys

@DennisOSRM
Copy link
Collaborator

Latest builds of the development branch are already getting published at http://build.project-osrm.org

@DennisOSRM
Copy link
Collaborator

@alex85k is this page on the wiki outdated?

@alex85k
Copy link
Contributor Author

alex85k commented Jun 11, 2014

I guess so. Can we delete most of its contents and replace with expanded extracts from https://gist.github.com/alex85k/8637217? Should the short instruction be put into https://github.com/DennisOSRM/Project-OSRM/wiki/Building-OSRM ?

@DennisOSRM
Copy link
Collaborator

Yep, doing that right now.

@DennisOSRM
Copy link
Collaborator

@DennisOSRM
Copy link
Collaborator

Wiki is updated

@alex85k
Copy link
Contributor Author

alex85k commented Jun 11, 2014

Thank you for fixing wiki!

I propose minor fixes for gist: https://gist.github.com/alex85k/8637217
You can also mention support files at http://build.project-osrm.org/ to simplify the process for users :)

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.

4 participants