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

Add windows support #707

Merged
merged 23 commits into from
Feb 25, 2023
Merged

Add windows support #707

merged 23 commits into from
Feb 25, 2023

Conversation

tdcosta100
Copy link
Collaborator

@tdcosta100 tdcosta100 commented Jan 19, 2023

I have specific needs to run Maplibre GL Native on Windows. So I created a port to Windows platform. It was inspired by Linux platform sources, and works in a similar way, creating some executables and the lib/node-vxx/mbgl.node modules. Therefore, Maplibre GL Native can be used with NodeJS in an Windows environment. I used TileServer-GL to test it, and it worked like a charm, the performance was similar to the Linux version. Build instructions are located at platform/windows/README.md. Remember to clone with --branch windows-support, because it have a new submodule. Or after changing to windows-support branch, you can init the platform/windows/vendor/vcpkg submodule manually. Feedback is welcome.

@wipfli
Copy link
Contributor

wipfli commented Jan 19, 2023

Oh beautiful, thanks @tdcosta100 for implementing windows support.

If we were to merge this, would you be interested in being the lead-maintainer for the Windows platform binding?

The usual work consists of handling dependency updates, reviewing pull requests, and helping people getting started with the platform binding...

@ntadej ntadej self-requested a review January 19, 2023 08:49
@tdcosta100
Copy link
Collaborator Author

tdcosta100 commented Jan 19, 2023

Oh beautiful, thanks @tdcosta100 for implementing windows support.

If we were to merge this, would you be interested in being the lead-maintainer for the Windows platform binding?

The usual work consists of handling dependency updates, reviewing pull requests, and helping people getting started with the platform binding...

I think I can try. I'm just afraid sometimes I will be with lots of demand from my job and not respond so quickly as I would like. Outside these situations, I think I can do the job. Can we make a trial period to see if I am a good fit for it?

Copy link
Collaborator

@ntadej ntadej left a comment

Choose a reason for hiding this comment

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

Thank you very much for providing this!

I provided some basic comments, I may look through the new files again later. To get this merged faster I would recommend to split the MR into smaller ones (e.g. fixing errors).

It would also be good to add a CI workflow to test your addition actually works.

CMakeLists.txt Outdated Show resolved Hide resolved
benchmark/CMakeLists.txt Outdated Show resolved Hide resolved
platform/windows/windows.cmake Outdated Show resolved Hide resolved
platform/windows/include/unistd.h Outdated Show resolved Hide resolved
@tdcosta100
Copy link
Collaborator Author

tdcosta100 commented Jan 19, 2023

Thank you very much for providing this!

I provided some basic comments, I may look through the new files again later. To get this merged faster I would recommend to split the MR into smaller ones (e.g. fixing errors).

It would also be good to add a CI workflow to test your addition actually works.

No worries about the changes. I can change everything, even open a new PR if it's better. I would propose this project to be evaluated before doing a PR, but I just didn't know where to do that hahaha. So if it's not mature enough, I will make the necessary changes before the merge.

Regarding the CI Workflow, I agree. I will try to setup the Windows workflow. I'm newbie to this, can I setup a workflow in a non-main branch?

@wipfli
Copy link
Contributor

wipfli commented Jan 19, 2023

can I setup a workflow in a non-main branch

Yes, like this for example

on:
  workflow_dispatch:
  pull_request:
    branches:
      - main
      - some_other_branch

@acalcutt
Copy link
Collaborator

If you need any help with getting this into the node-ci/build, let me know. It would be nice to add node windows support.

If you modify the node-ci workflow in the PR it is already set to run on pull request, so I don't think you would need to modify the branches it runs on. (at least that was how I was testing it before it got merged)

@tdcosta100
Copy link
Collaborator Author

tdcosta100 commented Jan 20, 2023

This is interesting! I will try it.

can I setup a workflow in a non-main branch

Yes, like this for example

on:
  workflow_dispatch:
  pull_request:
    branches:
      - main
      - some_other_branch

Nice, thanks!

@tdcosta100
Copy link
Collaborator Author

Once all pending changes to prepare for this PR are finally done (that is, changes that are not directly related with Windows, like Khronos definitions, and implicit casts), I'm resuming this PR. I converted it to draft in order to test actions for NodeJS on Windows platform. I didn't include actions yet, but once all tests passed, I will focus that.

@tdcosta100 tdcosta100 force-pushed the windows-support branch 4 times, most recently from cce2e3b to eccafae Compare February 11, 2023 21:41
@acalcutt
Copy link
Collaborator

acalcutt commented Feb 11, 2023

I spent some time working with @tdcosta100 to try and get this integrated into the 'node-ci' and 'node-release' workflows

We were able to get it all working, but I wanted to note a few issues we had along the way.

1.) When checking out the the repository on the github windows runner, I ran into path limit issues with some vendor test files. To get around this, I took a solution from the 'qt-ci', and did a shallow checkout and then pull the submodules after. This gets around the issue, but you can still see some errors relating to the paths that don't affect the build.

2.) To get ccache working in windows, I again pulled from the qt-ci, and used that to simplify our ccache use here (and make it work with windows).

3.) We initially had some issue with build type not being release, which made it this require c++ development dlls. we fixed that issue, but it should still be noted this requires c++ redistributable as a pre-req

Other than that, I was able to ci/release workflow that works.

I made a test release at
NPM: "@acalcutt/maplibre-gl-native": "6.1.0-pre.4"
https://github.com/acalcutt/maplibre-gl-native/releases/tag/node-v6.1.0-pre.4

and a tileserver-gl that uses it at
https://github.com/acalcutt/tileserver-gl/tree/windows_test

With that I was able to confirm this works!
image
image

Great work @tdcosta100

@tdcosta100
Copy link
Collaborator Author

tdcosta100 commented Feb 11, 2023

A huge THANK YOU to @acalcutt, who spent so many hours trying to adjust node-ci and node-release workflows, and succeeding at this painful job.

Regarding the too long path issues, I confirm it's resolved, at least when cloning submodules (recursively!). I don't know how tests will behave because I didn't let a test job finish yet, but I hope in the next few hours we will confirm if it works correctly.

@tdcosta100 tdcosta100 force-pushed the windows-support branch 3 times, most recently from e1f0fb5 to 1c38aba Compare February 14, 2023 04:12
@tdcosta100
Copy link
Collaborator Author

Finally got Windows tests working. Thank you @acalcutt again, for helping with the workflow.

@ovivoda ovivoda self-requested a review February 16, 2023 14:14
CMakeLists.txt Outdated Show resolved Hide resolved
expression-test/filesystem.hpp Outdated Show resolved Hide resolved
platform/node/cmake/module.windows.cmake Outdated Show resolved Hide resolved
platform/node/cmake/module.windows.cmake Outdated Show resolved Hide resolved
@tdcosta100
Copy link
Collaborator Author

Could you update the root README.md:

- [⭐️ Android](platform/android/README.md) 
- [⭐️ iOS](platform/ios/platform/ios/README.md)
- [GLFW](platform/glfw)
- [Linux](platform/linux/README.md)
- [Node.js](platform/node/README.md)
- [Qt](platform/qt/README.md)
- [Windows](platform/windows/README.md)
- [macOS](platform/ios/platform/macos/README.md)

Done!

README.md Show resolved Hide resolved
@acalcutt acalcutt merged commit f06f608 into maplibre:main Feb 25, 2023
tdcosta100 added a commit to tdcosta100/maplibre-native that referenced this pull request Feb 25, 2023
acalcutt pushed a commit that referenced this pull request Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants