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

Mingw cross compiler support. #99

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

zombieCraig
Copy link

Added host detection in SConstruct and set the flags for mingw++ compiling for linux and osx for compiling windows.

@beniwtv
Copy link
Collaborator

beniwtv commented Jul 16, 2020

Hey I've been meaning for some time to implement this myself, so thanks for working on this!

A couple of comments:

  • I would leave the platform= argument as it works currently, as that is used this way in other Godot-related projects (and Godot itself), so it would still be the same for us and not break and expectations / current scripts people may have
  • Instead, I suggest specifying the target platform as a target_platform= argument, if it is not given assume the target is the same as the current platform

:)

@BastiaanOlij
Copy link
Member

@beniwtv I don't think @zombieCraig is changing that behavior, whats new here is that you can perform the windows platform build on a non windows platform and therefor require to find out the host platform.

Godot I believe does the same for its cross compiling. I've never experimented with it myself

I need to take a closer look but if my understanding is correct I'm happy to merge this in. My only preliminary remark is whether it makes sense to check the host platform so early as its now also checked when you're compiling a linux build on linux, not that it would do any harm.

@zombieCraig
Copy link
Author

zombieCraig commented Jul 24, 2020 via email

@lyuma
Copy link
Contributor

lyuma commented Aug 1, 2020

I'd like to put in a vote for this too!

I actually independently made an almost identical change, available here:
lyuma@a71fde0
Only main difference is I moved host_platform detection into the windows case.

I think it's a good approach. Tested my variant on Linux (both native linux and mingw64) and on windows (msvc 2017)

@beniwtv
Copy link
Collaborator

beniwtv commented Aug 2, 2020

Ah yes I must have mistaken that variable, sorry, I got mixed up and thought platform was being overwritten :)
Looks good to me then.

@beniwtv
Copy link
Collaborator

beniwtv commented Aug 2, 2020

Hi managed to test this successfully with two small issues:

  1. In line 63 and 66 scons was complaining about mixed tabs and spaces for indentation
  2. The resulting file generated libgodot_openvr.so as name, even though according to file it is a proper Windows dll file.

:)

@zombieCraig
Copy link
Author

Oh no! 🤦 I can't tell you how many times python has bitten me for things like that. My IDE is VI, and I only usually code python patch others code. Needless to say, whitespace programming languages and myself do not get along. I'll fix that and resubmit.

@lyuma
Copy link
Contributor

lyuma commented Aug 3, 2020

Note:
If you build with MinGW (gcc or llvm variants), you will run into this bug. ValveSoftware/openvr#103

I'm going to see if the proposed fix works, running this script on openvr.h to generate a compatible version: https://gist.github.com/tunabrain/1fc7a4964914d61b5ae751d0c84f2382

I'll let you know if I am able to make headway on this crash, but basically OpenVR is designed using a C++ ABI and it's a bit scary to link code made with two different compilers

Just a heads-up so you don't spend lots of time debugging crashes.

Also, to workaround the .so problem, add this into the if statement:

    env['target_name'] += ".dll"

I have a complete diff if you want it, which includes llvm-mingw support, PDB generation using / -gcodeview in CCFLAGS and "-Wl,-pdb=" in LINKFLAGS, which I needed to figure out the above crash :-p

@beniwtv
Copy link
Collaborator

beniwtv commented Aug 3, 2020

Thanks for the heads up on the crash!

We did link LLVM with GCC compiled things when GCC was causing a crash before, and there weren't any issues, but I guess you never know!

@lyuma
Copy link
Contributor

lyuma commented Aug 3, 2020

@beniwtv Got it working!!

I have pushed my version of wrap_mingw.py here:
https://github.com/lyuma/godot_openvr/blob/mingw/wrap_openvr.py

You can see my commits here: https://github.com/lyuma/godot_openvr/tree/mingw
It includes llvm-mingw support and I get nice .pdb files.

(Note, requires a patch to replace env = DefaultEnvironment with env = Environment(ENV=os.environ) in SConstruct, but that's a different request for a different day.)

Here are the commands I am running from my CI system:

python wrap_openvr.py
rm -f demo/addons/godot-openvr/bin/win64/libgodot_openvr.dll
PATH=/opt/llvm-mingw/bin:$PATH scons werror=no platform=windows target=release -j`nproc` use_lto=no deprecated=no use_mingw=yes use_llvm=yes use_lld=yes use_thinlto=yes LINKFLAGS=-Wl,-pdb= CCFLAGS='-g -gcodeview' debug_symbols=no

@lyuma
Copy link
Contributor

lyuma commented Aug 4, 2020

Good news, Benedikt has updated wrap_openvr.py to have a MIT license:
https://gist.github.com/tunabrain/1fc7a4964914d61b5ae751d0c84f2382

would a PR for this be helpful? I'd be curious if anyone else has run into the issues I had when using a HMD with mingw-compiled libgodot_openvr.dll (the crash only happens when actually running in VR)

@beniwtv
Copy link
Collaborator

beniwtv commented Aug 4, 2020

We'd need to run this file either manually or via scons during (before) cross-compile right?
I have compiled the .dll, but as I have no Windows I am unable to test if it crashes without this wrapper, but I guess if it does for your build mine would be the same.

The Python script creates a wrapper, that uses OpenVR's C code instead of the C++ one, so we are using the C ABI which is more compatible, as I understand it.

@BastiaanOlij Do you think this would be an acceptable approach for us?

@BastiaanOlij
Copy link
Member

If we can script it so it only does that when the mingw build is run, I don't have anything against that solution. It's no different than the extra step done on MacOS

@BastiaanOlij
Copy link
Member

This needs a rebase

@beniwtv
Copy link
Collaborator

beniwtv commented Nov 16, 2020

Since we now have automated builds for every platform, is this still something we want to pursue?

@zombieCraig
Copy link
Author

zombieCraig commented Nov 16, 2020 via email

@lyuma
Copy link
Contributor

lyuma commented Nov 17, 2020

@beniwtv Apologies for being a bit out-of-the-loop on this, but what exactly is the new automated builds approach?

If this is a reference to GitHub Actions, unfortunately that is not an acceptable approach for us. GitHub Actions is proprietary (even if free-as-in-beer), and requirements for my project are that we be able to use our own hardware to compile releases... Running Visual Studio emulated with Wine might be possible, but it sounds like a maintenance nightmare.

@zombieCraig mentions Linux. If there is a fix for Linux based (non-MS VIsual Studio) compilers such as mingw GCC, llvm-mingw, would it be possible to point me in that direction?

@beniwtv
Copy link
Collaborator

beniwtv commented Nov 27, 2020

@lyuma Yes, the automatic builds won't then fulfill those requirements then -> so we still need this. Feel free to rebase & resend :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants