node-mapnik icon indicating copy to clipboard operation
node-mapnik copied to clipboard

Enable all platforms again

Open mathisloge opened this issue 4 years ago • 46 comments

So this PR adds a way to build for all platforms that are supported by mapnik (including windows) Currently the whole dependency setup is done via vcpkg. I recommend this for windows, because, as #848 stated, it would take a lot of effort to pull in all the dependencies with a script. On linux and osx the system libraries should be used. This splitting is generally easy. On windows the submodule would be cloned or, as it is also possibile, added via FetchContent. Otherwise mapnik would be found via pkg-config or cmakes find_package.

~~However since this will use cmake as a build system i had to remove node-gyp. I think it will need discussion if you want to pursude this path.~~ This will use cmake-js as the build system and node-pre-gyp for packaging and publishing.

Depends on:

  • https://github.com/mapnik/mapnik/pull/4191
  • https://github.com/microsoft/vcpkg/pull/18771
  • https://github.com/microsoft/vcpkg/pull/18849
  • ~~currently there is a known issue that happens when installing mapnik in manifest mode. The proj port will inject a invalid include path which get carried into mapniktargets.cmake.~~ RESOLVED

~~if someone wants to try this PR, the file build\vcpkg_installed\<your_triplet>\share\mapnik\MapnikTargets.cmake needs a patch at line 57 with [...]vcpkg_installed/include to [...]vcpkg_installed/<your_triplet>/include. You will need to build manually with cmake-js build--CDCMAKE_TOOLCHAIN_FILE="./vcpkg/scripts/buildsystems/vcpkg.cmake" --target node-mapnik and after the first build file, apply the described workaround.~~ RESOLVED

Todo:

  • [x] create tarball with plugins etc.
  • [ ] auto generate mapnik_settings.json with the correct values
  • [x] as soon as all PRs are merged (vcpkg related) switch vcpkg submodule to microsoft/vcpkg
  • [x] ~~maybe there is a way to use node-pre-gyp in conjunction with cmake-js( build with cmake, publish and package with node-pre-gyp)~~ so building with cmake-js and package & publish with node-pre-gyp works pretty well
  • [x] ogr and gdal currently don't work. Need to investigate why (only tested on windows). There might be a missing hidden dependency. ~could narrow down the problem: somehow the libpq could not be imported.~ was just a missing PATH entry
  • [ ] windows unicode need some reworks. (think this should be fixed when the CI changes are getting merged into mapnik@master)

prebuild binaries

For development and testing purposes, there is a devbuild npm package: https://www.npmjs.com/package/@mathisloge/mapnik

Currently there should be published packages for windows and linux.

integration tests

Node.js CI Simple integration test with windows and linux each with node versions 12, 14 and 16 https://github.com/mathisloge/node-mapnik-test

mathisloge avatar Jul 02 '21 15:07 mathisloge

I will wait for some feedback before putting more effort into it.

mathisloge avatar Jul 02 '21 15:07 mathisloge

mainly seeking some feedback from you @artemp :D

mathisloge avatar Jul 02 '21 18:07 mathisloge

Just curious if this might be merged anytime in the near future?

JoshuaHintze avatar Oct 11 '21 19:10 JoshuaHintze

@GimpMaster I'm first need to merge https://github.com/mapnik/mapnik/pull/4252 as that resolves some issues. Currently ogr and gdal plugins can't be loaded. I don't know exactly what the reason is, because all dependencies are resolved correctly and a normal mapnik application can also load those plugins. So I need to investigate that further.

mathisloge avatar Oct 11 '21 19:10 mathisloge

thank you @mathisloge - Could I just npm add your git repo and try on windows? or is there more to it than that?

JoshuaHintze avatar Oct 11 '21 20:10 JoshuaHintze

@GimpMaster I know that @artemp haven't pushed proj6 to master yet. So expect that a lot of tests would fail. Also i haven't test all things thoroughly, since it is very much a WIP.

However, if you don't need gdal or ogr, you could clone the repo (and submodules!) and perform the needed install steps. It should be as little as:

  • npm install
  • npm run build

You need a compiler for c++, preferably msvc 2019 and cmake >= 3.21, 3.20 might also be ok. Havent tested the lower versions yet.

I would appreciate any feedback if you build and use it :)

mathisloge avatar Oct 11 '21 20:10 mathisloge

@GimpMaster just tested it to install just with npm install https://github.com/mathisloge/node-mapnik this does not download the submodules. However you could also clone https://github.com/mathisloge/node-mapnik and run npm install and npm run build. After that you should be able to add it to your project with npm install <path_to_node-mapnik>. (At least that worked for me)

mathisloge avatar Oct 11 '21 20:10 mathisloge

Great,

I'll give it a try tonight.

I'll give any feedback I find.

Thanks!

On Mon, Oct 11, 2021 at 2:50 PM Mathis @.***> wrote:

@GimpMaster https://github.com/GimpMaster just tested it to install just with npm install https://github.com/mathisloge/node-mapnik this does not download the submodules. However you could also clone https://github.com/mathisloge/node-mapnik and run npm install and npm run build. After that you should be able to add it to your project with npm install <path_to_node-mapnik>.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mapnik/node-mapnik/pull/976#issuecomment-940431225, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB3UXWEZ56E5TAWGYKISV5LUGNE2HANCNFSM47W574WA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

JoshuaHintze avatar Oct 11 '21 20:10 JoshuaHintze

@mathisloge - Well got closer. Installed your repo. initialized git sub modules. Updated CMake to latest version. Had to also install Windows SDK 8.1. Then finally onto vcpkg.

vcpkg installed 136 packages, compiled for 1.5 hours! and on the last one mapnik:x64-windows it failed

Error: Building package mapnik:x64-windows failed with: BUILD_FAILED Please ensure you're using the latest portfiles with .\vcpkg update, then submit an issue at https://github.com/Microsoft/vcpkg/issues including: Package: mapnik:x64-windows Vcpkg version: 2021-09-10-2059ef11aa6067e6f59b0d939c5d17e3c5c47d3e

Still trying to look through logs why that failed.

JoshuaHintze avatar Oct 12 '21 05:10 JoshuaHintze

looking at the log this is the error I see error C2039: 'min': is not a member of 'std'

Trying to compile with VS2017

JoshuaHintze avatar Oct 12 '21 05:10 JoshuaHintze

Ah okay. Yeah I think usera had reported issues with <vs2019 previously. Since the windows headers are defining min and max as macros and collide with anything that named min or max.

mathisloge avatar Oct 12 '21 05:10 mathisloge

Are you using VS2019?

I thought node installs typically install VS2017 for node-gyp

On Mon, Oct 11, 2021 at 23:55 Mathis @.***> wrote:

Ah okay. Yeah I think usera had reported issues with <vs2019 previously. Since the windows headers are defining min and max as macros and collide with anything that named min or max.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mapnik/node-mapnik/pull/976#issuecomment-940687868, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB3UXWE2DFNW3TJTAVKRC7DUGPEW5ANCNFSM47W574WA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

JoshuaHintze avatar Oct 12 '21 05:10 JoshuaHintze

If you have a chance to update to vs2019 that would be the fastest way.

I could add a compile define NOMINMAX to mapnik for <vs2019. But that would need some testing from my side, as I would need to test with vs2017 etc. if there aren't any dependencies that depend on the macro.

mathisloge avatar Oct 12 '21 06:10 mathisloge

I'm using vs2019. This PR uses node-pre-gyp just for the packaging. The build will be invoked through cmake-js

mathisloge avatar Oct 12 '21 06:10 mathisloge

I think it would be good to get it working on vs2017. I think when you default install NodeJS LTS on windows through the MSI installer it installs vs2017.

On Tue, Oct 12, 2021 at 00:07 Mathis @.***> wrote:

I'm using vs2019. This PR uses node-pre-gyp just for the packaging. The will be invoked through cmake-js

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mapnik/node-mapnik/pull/976#issuecomment-940693722, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB3UXWBPCVX2SYWKGWX4XLDUGPGCJANCNFSM47W574WA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

JoshuaHintze avatar Oct 12 '21 14:10 JoshuaHintze

I will look into it. But it will be at a later time as I first need to get all other things done. So currently only vs2019 and vs2022. But vs2017 is a valid target and needs to be fixed

mathisloge avatar Oct 12 '21 15:10 mathisloge

Anything I can do to help? I’m not familiar with node-gyp but I was a c++ developer for 10 years however the last six has been NodeJS and react.

On Tue, Oct 12, 2021 at 09:24 Mathis @.***> wrote:

I will look into it. But it will be at a later time as I first need to get all other things done. So currently only vs2019 and vs2022. But vs2017 is a valid target and needs to be fixed

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mapnik/node-mapnik/pull/976#issuecomment-941117829, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB3UXWD57WIU2S6L3BORFJTUGRHMTANCNFSM47W574WA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

JoshuaHintze avatar Oct 12 '21 15:10 JoshuaHintze

Just to note: The build isn't invoked with node-gyp. Instead it uses cmake. Any limitations related with node-gyp don't apply here.

So the mapnik build error is related to mapnik itself. Someone had already begin with vs2017. https://github.com/mapnik/mapnik/pull/4184 But I don't know if those are really the only two occurances where the macros would collide with the min/max usage.

I'm working currently on https://github.com/mapnik/mapnik/pull/4252 which fixes some problems i have in this PR.

And then the outstanding leftover issues with this are the plugins ogr and gdal which can't be loaded.

mathisloge avatar Oct 12 '21 15:10 mathisloge

@mathisloge - I played around a little bit more with this tonight trying to get 2017 working. As far as I can tell right now the only two std::min / std::max errors are in

include/mapnik/util/char_array_buffer.hpp

line 74, 81, 82, 83

current_ = std::min(begin_ + off, end_)

I tried manually modifying this file but every time I do an npm run build vcpkg gets a clean copy and overwrites my local changes.

Any thoughts on how I can change vcpkg to pull from a forked repo for mapnik?

Thanks!

JoshuaHintze avatar Oct 15 '21 03:10 JoshuaHintze

@GimpMaster the quickest way would be to modify vcpkg/ports/mapnik/portfile.cmake. Just replace

vcpkg_from_github(
    OUT_SOURCE_PATH SOURCE_PATH
    REPO mapnik/mapnik
    REF 0edb018465790cd156d6849557fa7fd568755ebb
    SHA512 cd6180b96dbfbc1bafbc8f2a4ff2091a0e8c0c42b0569ef83640ad1f5766ff74ea95f8c6cef53f879396cbceebc6396c1a977676eca67c401b8e145a9ceae7e2
    HEAD_REF master
)

with set(SOURCE_PATH <absolute path to your local fork>)

or if you want to use your github repo, replace REPO mapnik/mapnik with REPO <your_name>/mapnik But you would need to update the REF to the latest commit and SHA512. The expected SHA512 will be printed out when building. So you could first change REPO and REF, build, copy expected SHA512 and replace it and build again. (The first build will fail after the download, so no long wait times :) )

mathisloge avatar Oct 15 '21 05:10 mathisloge

So I went with your suggestion of set(SOURCE_PATH )

I know it is correctly pointing to the path because the first time I had the path wrong and it failed immediately. I then fixed the absolute path and it takes a lot longer but ultimately fails. It looks like it doesn't even try to compile. It runs all the cmake checks to make sure it has all the libraries but then it ultimately ends saying

Build files have been written to ....

This is where I get to when running npm install.

See attached config-x64-windows-out.log file

install-x64-windows-dbg-out.log

Detecting compiler hash for triplet x64-windows...
The following packages will be built and installed:
    mapnik[cairo,core,grid-renderer,input-csv,input-gdal,input-geobuf,input-geojson,input-ogr,input-pgraster,input-postgis,input-raster,input-shape,input-sqlite,input-topojson,jpeg,png,proj,svg-renderer,tiff,utility-mapnik-index,utility-shapeindex,webp]:x64-windows -> 2021-10-06
Restored 0 packages from C:\Users\Josh\AppData\Local\vcpkg\archives in 1.209 ms. Use --debug to see more details.
Starting package 1/1: mapnik:x64-windows
Building package mapnik[cairo,core,grid-renderer,input-csv,input-gdal,input-geobuf,input-geojson,input-ogr,input-pgraster,input-postgis,input-raster,input-shape,input-sqlite,input-topojson,jpeg,png,proj,svg-renderer,tiff,utility-mapnik-index,utility-shapeindex,webp]:x64-windows...
-- Configuring x64-windows
-- Building x64-windows-dbg
CMake Error at scripts/cmake/vcpkg_execute_build_process.cmake:155 (message):
    Command failed: "C:/Program Files/CMake/bin/cmake.exe" --build . --config Debug --target install -- -v -j9
    Working Directory: H:/Fast_Dev/imsar/node-mapnik/vcpkg/buildtrees/mapnik/x64-windows-dbg
    See logs for more information:
      H:\Fast_Dev\imsar\node-mapnik\vcpkg\buildtrees\mapnik\install-x64-windows-dbg-out.log

Call Stack (most recent call first):
  H:/Fast_Dev/imsar/node-mapnik/build/vcpkg_installed/x64-windows/share/vcpkg-cmake/vcpkg_cmake_build.cmake:102 (vcpkg_execute_build_process)
  H:/Fast_Dev/imsar/node-mapnik/build/vcpkg_installed/x64-windows/share/vcpkg-cmake/vcpkg_cmake_install.cmake:41 (vcpkg_cmake_build)     
  ports/mapnik/portfile.cmake:65 (vcpkg_cmake_install)
  scripts/ports.cmake:141 (include)


Error: Building package mapnik:x64-windows failed with: BUILD_FAILED
Please ensure you're using the latest portfiles with `.\vcpkg update`, then
submit an issue at https://github.com/Microsoft/vcpkg/issues including:
  Package: mapnik:x64-windows
  Vcpkg version: 2021-09-10-2059ef11aa6067e6f59b0d939c5d17e3c5c47d3e

Additionally, attach any relevant sections from the log files above.

I'm not sure how to get it passed this step and to actually try compiling my local fork. Appreciate any next steps to try.

JoshuaHintze avatar Oct 15 '21 16:10 JoshuaHintze

It looks like it is still not correctly wrapped. Have you wrapped them like this: (std::min)(x, y); ? so that the function-like macro won't be applied?

H:\Fast_Dev\imsar\mapnik\include\mapnik/util/char_array_buffer.hpp(74): error C2039: 'min': is not a member of 'std'
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\iterator(16): note: see declaration of 'std'
H:\Fast_Dev\imsar\mapnik\include\mapnik/util/char_array_buffer.hpp(74): error C3861: 'min': identifier not found
H:\Fast_Dev\imsar\mapnik\include\mapnik/util/char_array_buffer.hpp(81): error C2039: 'min': is not a member of 'std'
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\iterator(16): note: see declaration of 'std'
H:\Fast_Dev\imsar\mapnik\include\mapnik/util/char_array_buffer.hpp(81): error C3861: 'min': identifier not found
H:\Fast_Dev\imsar\mapnik\include\mapnik/util/char_array_buffer.hpp(82): error C2039: 'min': is not a member of 'std'
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\iterator(16): note: see declaration of 'std'
H:\Fast_Dev\imsar\mapnik\include\mapnik/util/char_array_buffer.hpp(82): error C3861: 'min': identifier not found
H:\Fast_Dev\imsar\mapnik\include\mapnik/util/char_array_buffer.hpp(83): error C2039: 'max': is not a member of 'std'
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\iterator(16): note: see declaration of 'std'
H:\Fast_Dev\imsar\mapnik\include\mapnik/util/char_array_buffer.hpp(83): error C3861: 'max': identifier not found

Is this H:\Fast_Dev\imsar the right repo?

mathisloge avatar Oct 15 '21 16:10 mathisloge

@mathisloge - I was able to get mapnik to build with VS2017. The issue wasn't the macro the issue was that std::min std::max is defined in

#include <algorithm>

I just needed to add the #include into two places.

src/webp_reader.cpp include/mapnik/util/char_array_buffer.hpp

After that it built successfully.

Currently trying to yarn add your node-mapnik to my project. Feels like its rebuilding it again now instead of using the binaries, but my CPU is humming so it must be building again.

JoshuaHintze avatar Oct 15 '21 19:10 JoshuaHintze

@mathisloge - So that all worked! I was able to install and use mapnik on windows with VS2017 compiling.

However I didn't realize that you had already mentioned this but I DO need gdal to work. I'm rastering geotiffs which uses the gdal library for rendering. I'm sure you have already seen this error

Mapnik LOG> 2021-10-15 13:58:43: Problem loading plugin library: H:\Fast_Dev\imsar\mapnik-test\node_modules\mapnik\lib\binding\napi-v3\mapnik\input\gdal.input (dlopen failed - plugin likely has an unsatisfied dependency or incompatible ABI)

Let me know if I can help on any of that too.

JoshuaHintze avatar Oct 15 '21 20:10 JoshuaHintze

Yeah. I'm working on it. I have narrowed it down to libpq. Need to attach windbg to it. To get more infos, why it fails to load and which symbols are missing.

Sorry for the inconvenience!

mathisloge avatar Oct 15 '21 20:10 mathisloge

@mathisloge - No inconvenience at all. You have been very helpful!

Two quick questions: When this is all done will there be a binary version that we can distribute so others don't need to go through all the build steps?

Do you want to submit the MR for mapnik for VS2017 compiling or do you want me to? Seems like you have some prior experience with them doing so, but I would be glad to if you don't.

JoshuaHintze avatar Oct 15 '21 20:10 JoshuaHintze

I try to get it ready as fast as possible. But I don't know how long I need to debug. The pr is ready to distribute binaries (npm run publish).

You can open the PR if you want :)

mathisloge avatar Oct 15 '21 20:10 mathisloge

@GimpMaster https://github.com/microsoft/vcpkg/pull/20824 vcpkg will soon be updated and has your fix.

mathisloge avatar Oct 18 '21 16:10 mathisloge

Awesome. GDAL is still not working correct? @mathisloge

JoshuaHintze avatar Oct 18 '21 16:10 JoshuaHintze

Yeah. I guess it is something with libpq. I haven't had so much time this weekend. hopefully I get some results in this week.

mathisloge avatar Oct 18 '21 17:10 mathisloge