-
Notifications
You must be signed in to change notification settings - Fork 240
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
[WIP] Windows platform support #1812
base: main
Are you sure you want to change the base?
Conversation
# A hack to suppress "already exists" error | ||
MKPATH_QUIET = 2>nul || VER>nul | ||
else | ||
# Civilized rest of the world |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂
Hey thanks for all the work on this port @tehKaiN ! Windows support has been missing for too long. I especially appreciate the documentation for different compiler toolchains. It's been difficult to find time for code review on this because our core team only has macs during working hours, but I will commit that this weekend I'm going to spend time trying this out and hopefully we can get this merged. |
How's it goin'? ;) |
https://github.com/tangrams/tangram-es/tree/windows-v2 My goal here has been to get the sample app working with Visual Studio - since that's the only build toolchain that I really use on Windows :D It doesn't seem impossible, but there still some build issues with the Mapbox Variant submodule I need to figure out. One thing that simplified the build process greatly for me was using Conan to manage library dependencies like Zlib and libCURL. On Windows the complexity of managing binary dependencies is great enough that something like Conan actually makes sense, and it worked exactly as advertised. |
In regards to merging these windows changes, I don't mind merging things as they are and continuing to work on the build configuration incrementally. This work is almost totally separate from the other platform targets, so there isn't much risk of breaking something else. I think it will be important to add a CI configuration (probably Appveyor) to check our Windows builds regularly. Otherwise, we're just going to break Windows all the time - since none of us maintainers work on Windows. @tallytalwar @hjanetzek Thoughts? |
FYI, I also started looking at this at my home pc. More eyes the better. Will update more this week. |
Ahh, just saw your mention @matteblair. We should give it a proper look before merging this in master, as we have spoken before, once in master it will be officially supported. Probably add some WIP/Beta with it in the windows platform readme. That being said, totally cool with this being merged, but maybe give me a week(ish) time to finish whats in here. For CI yes, Appveyor is good, I think we did play with it for the previous attempt of merging windows in. Will also be good to add unit tests support for the same and not just the builds. |
I'm fine with that as long as it gets merged eventually. ;) I think I'm done here. Is there anything else I should do for this PR? |
Tiny update. Thanks @tehKaiN for documenting this PR so well, specially all the possible windows platform toolchains. I have the basic setup done on my home machine and plan to finish reviewing this the following week. I agree with @matteblair about having dependency management (can be added as a subsequent PR) and CI (with this PR or immediately after this, though should not be too hard). More to followup next week on this. |
Guys? |
I see that now there are conflicts in some files. If anyone is willing to review my changes and eventually merge them I can fix them for you. |
@tehKaiN @matteblair @tallytalwar I can review this branch if there is still interest in getting windows platform in main. |
@karimnaaji go ahead please :D. |
@karimnaaji the file structure and build system has changed a bit since beginning of this pull request. You want me to make this stuff compatible with master, or would you like to do it yourself? |
@tehKaiN no it's ok since I think the consensus was to use Conan so I'll directly start from main to use it for the build, and add your code changes on top. |
@karimnaaji I've added required changes on top of recent master since I need this lib for my company's app and I like to be sync'd with latest stuff. You can look at it here: https://github.com/tehKaiN/tangram-es/tree/windows-v3 Unfortunately there are some crashes as I've recently described in #1569 which weren't present before. I was hoping for wxWidgets platform support which I have implemented too, but I think incorporating it to your repo won't work in the long run - I'll refactor it as a lib with libtangram as dependency when this pull request is over. |
@tehKaiN sounds good! So far I've rebased the branch locally, and tried a few things:
|
@karimnaaji perhaps it'd be simpler to make it work with bare CMake in same way as rest of platforms are supported. You can resolve CURL/zlib dependencies by building them using CMake and then using Having libs installed in predefined directory simplifies things a lot, just don't use default install path (Program Files) as it requires elevated privileges. I think it's a way to go and I'd really like to have libtangram properly installed in similar manner so that other projects can use it without incorporating it as submodule and directly building using project's build process. And yes, forget about MSVC for now as its support for latest C++ is a pos. Even Microsoft adds Clang to Visual Studio installers, and there's no coincidence in that. ;) Unfortunately Clang on Windows uses MSVC compatibility mode instead of GCC one as on rest of platforms, which I haven't figured out how to disable. |
Not ready yet!