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

Make rwgame build and run on windows #130

Merged
merged 3 commits into from
Jul 30, 2016
Merged

Conversation

haphzd
Copy link
Contributor

@haphzd haphzd commented Jun 4, 2016

Hi. I'm kinda new at all of this, so there are couple things I am not sure about.

  1. Msdirent header (how to distribute it? where should it be in the project tree?)
  2. Some defines in the windows platform headers collide with enum identifiers and class fields/members names. My approach at solving this is questionable (a header with #undef macros), but it's the only one that worked for me.
    I haven't tried to build tools and tests, just rwgame/rwengine/rwlib.

@JayFoxRox
Copy link
Collaborator

JayFoxRox commented Jun 4, 2016

Ignore this. We are going to have a better platform abstraction later - this is fine for now.


  1. I'd suggest https://github.com/tronkko/dirent as submodule (which seems to be the original author)
    Another solution is to change respective code paths to use windows functions directly or to abstract the entire file-io in general.
    (Also note that mingw should have its own dirent implementation - we'd only need above solution for MS VS)
  2. Where do these defines come from? Maybe MS have a way to disable them?
    I'd say keep #undef for now and later attempt to get platform specific includes out of the files where we have enums etc. so this couldn't happen in the first place.

I'll probably review this too but I'm not sure when I'll find the time to do it.

@JayFoxRox
Copy link
Collaborator

Linking this to #117

@@ -25,6 +25,9 @@ option(ENABLE_PROFILING "Enable detailed profiling metrics")
#
# Build configuration
#
if(NOT CMAKE_BUILD_TYPE)
set(CMAKE_BUILD_TYPE Release CACHE STRING "Choose the type of build, options are: Debug Release" FORCE)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

Does this still work for non-makefile generators like Visual Studio?

@danhedron
Copy link
Member

Sorry for the delay.

I'd prefer abstracting the file-system interaction to avoid needing platform specific fix ups in the game code itself. This could live in rwlib where the damage from system headers can be contained to a single compilation unit.

@danhedron
Copy link
Member

That being said I think abstracting the file system access would be outside the scope of this PR. If there is nothing else you'd like to add to it then I'll accept on the basis that file system access will be changed in future.

@danhedron
Copy link
Member

There is currently a merge conflict with master; could you re-base these changes?

@@ -0,0 +1,3 @@
#undef IGNORE
#undef near
#undef far
Copy link
Member

Choose a reason for hiding this comment

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

What do these collide with? Could needing these undefinitions be avoided by including different windows headers or by changing the name that conflict?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IGNORE is defined in winbase.h, near and far are defined in windef.h.

Copy link
Member

Choose a reason for hiding this comment

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

Those sound required. The windows headers should definitely be isolated from the rest of the code eventually, but this header is an OK fix for now.

@JayFoxRox
Copy link
Collaborator

JayFoxRox commented Jul 6, 2016

Sorry to bother you again, but we accidently caused another merge conflict just before wanting to merge this.
We can't rebase it for you as we don't run Windows. So we can't test it again.

Could you rebase & test again?

We'll merge it then and we fix any remaining issues (if there are any) later.
(Also, we are interested in setting up AppVeyor CI, soon)

You can also talk to us on IRC in #openrw on freenode for a more interactive conversation :)
(Where we discussed this PR a couple of times because we feel stupid about the repeated merge conflicts)

@haphzd
Copy link
Contributor Author

haphzd commented Jul 6, 2016

I have rebased and built it locally, but now it doesn't run so good. I'll join IRC sometime soon, because it seems I can't figure out the cause of all them segfaults.

@JayFoxRox
Copy link
Collaborator

JayFoxRox commented Jul 8, 2016

As you didn't join IRC yet I'll spoil the bug hunting fun here instead: OpenRW is currently very crashy due to a bug in the physics code; there is another PR to fix it, but it's still WIP (ever since adding parked cars last week, so test mode should be slightly less crashy)
It also means that the cause for the segfaults is probably not in your PR.

@darkf
Copy link
Collaborator

darkf commented Jul 27, 2016

Works pretty well, only problem I had was with mman.h being copied to $MINGW64_ROOT/include/sys (I guess sys wasn't already a directory, so it just copied it to that filename.)

It also needs a rebase.

@haphzd haphzd force-pushed the mingw-w64 branch 2 times, most recently from f9d566e to 1ad5bf7 Compare July 27, 2016 18:10
@darkf
Copy link
Collaborator

darkf commented Jul 28, 2016

re WinSock: you need to WSAStartup before you use any WinSock functions. Can always do that later I suppose.

Other than that, LGTM. Can we get this merged soon?

@danhedron
Copy link
Member

Keen to get more developers looking at windows and in general. Further issues can be resolved with more PRs.

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