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

Build failing for node v6.5.0 on windows #691

Closed
springmeyer opened this issue Sep 5, 2016 · 21 comments
Closed

Build failing for node v6.5.0 on windows #691

springmeyer opened this issue Sep 5, 2016 · 21 comments
Milestone

Comments

@springmeyer
Copy link
Member

Been trying to get node v6 working on windows. What I've done is:

Errors are new and curious:

mapnik-wkt.lib(mapnik_wkt_generator_grammar.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MD_DynamicRelease' doesn't match value 'MT_StaticRelease' in mapnik_logger.obj [C:\projects\node-mapnik\build\mapnik.vcxproj]
mapnik-json.lib(mapnik_geometry_to_geojson.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MD_DynamicRelease' doesn't match value 'MT_StaticRelease' in mapnik_logger.obj [C:\projects\node-mapnik\build\mapnik.vcxproj]
mapnik-json.lib(mapnik_json_generator_grammar.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MD_DynamicRelease' doesn't match value 'MT_StaticRelease' in mapnik_logger.obj [C:\projects\node-mapnik\build\mapnik.vcxproj]
mapnik-json.lib(mapnik_json_feature_grammar.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MD_DynamicRelease' doesn't match value 'MT_StaticRelease' in mapnik_logger.obj [C:\projects\node-mapnik\build\mapnik.vcxproj]
mapnik-json.lib(generic_json.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MD_DynamicRelease' doesn't match value 'MT_StaticRelease' in mapnik_logger.obj [C:\projects\node-mapnik\build\mapnik.vcxproj]
mapnik-json.lib(mapnik_json_geometry_grammar.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MD_DynamicRelease' doesn't match value 'MT_StaticRelease' in mapnik_logger.obj [C:\projects\node-mapnik\build\mapnik.vcxproj]
mapnik-json.lib(mapnik_json_positions_grammar.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MD_DynamicRelease' doesn't match value 'MT_StaticRelease' in mapnik_logger.obj [C:\projects\node-mapnik\build\mapnik.vcxproj]
libboost_thread-vc140-mt-1_61.lib(tss_pe.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MD_DynamicRelease' doesn't match value 'MT_StaticRelease' in mapnik_logger.obj [C:\projects\node-mapnik\build\mapnik.vcxproj]
libboost_thread-vc140-mt-1_61.lib(thread.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MD_DynamicRelease' doesn't match value 'MT_StaticRelease' in mapnik_logger.obj [C:\projects\node-mapnik\build\mapnik.vcxproj]
libboost_system-vc140-mt-1_61.lib(error_code.obj) : error LNK2038: mismatch detected for 'RuntimeLibrary': value 'MD_DynamicRelease' doesn't match value 'MT_StaticRelease' in mapnik_logger.obj [C:\projects\node-mapnik\build\mapnik.vcxproj]
     Creating library C:\projects\node-mapnik\build\Release\mapnik.lib and object C:\projects\node-mapnik\build\Release\mapnik.exp
LINK : warning LNK4098: defaultlib 'MSVCRT' conflicts with use of other libs; use /NODEFAULTLIB:library [C:\projects\node-mapnik\build\mapnik.vcxproj]
mapnik-wkt.lib(mapnik_wkt_generator_grammar.obj) : error LNK2001: unresolved external symbol __imp__modf [C:\projects\node-mapnik\build\mapnik.vcxproj]
mapnik-json.lib(mapnik_json_generator_grammar.obj) : error LNK2001: unresolved external symbol __imp__modf [C:\projects\node-mapnik\build\mapnik.vcxproj]
MSVCRT.lib(_chandler4gs_.obj) : error LNK2001: unresolved external symbol __except_handler4_common [C:\projects\node-mapnik\build\mapnik.vcxproj]
libboost_thread-vc140-mt-1_61.lib(thread.obj) : error LNK2001: unresolved external symbol __imp___gmtime64 [C:\projects\node-mapnik\build\mapnik.vcxproj]
libboost_system-vc140-mt-1_61.lib(error_code.obj) : error LNK2001: unresolved external symbol __imp__strerror [C:\projects\node-mapnik\build\mapnik.vcxproj]
C:\projects\node-mapnik\build\Release\mapnik.node : fatal error LNK1120: 4 unresolved externals [C:\projects\node-mapnik\build\mapnik.vcxproj]
gyp ERR! build error 
gyp ERR! stack Error: `msbuild` failed with exit code: 1
@springmeyer springmeyer added this to the v3.5.14 milestone Sep 5, 2016
@springmeyer
Copy link
Member Author

Oh, this is likely because I'm using upstream node directly and not our fork. Since we've usually changed the runtime to shared in our patches.... like https://github.com/mapbox/node/pull/9/files. Now, for node v4/5 I think I did not change this, or it looks like I may have missed it. So I forgot about it for node v6. So, perhaps we do still need to fork and build with 'RuntimeLibrary': 0 -> 'RuntimeLibrary': 2...

Looking through the node v6 build scripts it looks like there is node_shared variable that toggles this: https://github.com/nodejs/node/blob/dfe1c7e9b809740bbc511a55c668f80a5f5e4c22/common.gypi#L82-L96. Not sure if it does the right thing (might do more than we want) but tried it in: mapbox/node-cpp11@ef06f92

/cc @BergWerkGIS

@springmeyer
Copy link
Member Author

hmm, after mapbox/node-cpp11@ef06f92 and restarting the appveyor job I'm still seeing the same error... @BergWerkGIS help :)

@springmeyer
Copy link
Member Author

made some progress today. Detailed at mapnik/mapnik#3510 (comment). But now blocked on this error I'm unsure the cause of:

Error: The specified module could not be found.
[00:04:22] 
[00:04:22] \\?\C:\projects\node-mapnik\lib\binding\node-v48-win32-x64\mapnik.node
[00:04:22] 

@wilhelmberg
Copy link
Contributor

wilhelmberg commented Sep 7, 2016

But now blocked on this error I'm unsure the cause of

This error check has been disabled thus masking that the build has failed and mapnik.node does not exist.

@wilhelmberg
Copy link
Contributor

WRT /MT vs. /MD for node.exe/.lib: This is how its done in windows-builds.

Patching the call to msbuild in node's vsbuild.bat to include this additional msbuild config fragment which forces /MD for the whole node.sln.

I suggest we carry the patch and the props file over to https://github.com/mapbox/node-cpp11/blob/master/windows/build_node.bat
What do you think?

@wilhelmberg
Copy link
Contributor

node-mapnik build is failing in the early stages already when npm is pulling down node headers and node.lib as the directory structure is not as expected and SHASUMS do not match:
https://ci.appveyor.com/project/Mapbox/node-mapnik/build/1.0.1563#L9050

The directory structure for node.exe/.lib on nodejs.org has changed since node@4.
This probably would have to be accounted for in https://github.com/mapbox/node-cpp11/blob/7154967dd0b87abefaae3172999b3d38e40ee9a1/windows/build_node.bat#L44-L57

@wilhelmberg
Copy link
Contributor

node-mapnik with [email protected] builds successfully (all tests pass) using nodejs's node:
https://ci.appveyor.com/project/Mapbox/node-mapnik/build/1.0.1565

@wilhelmberg
Copy link
Contributor

wilhelmberg commented Sep 7, 2016

A hint that failing node-mapnik builds are linked to using Mapbox's node (different directory structure, different SHASUMS, ...).
https://ci.appveyor.com/project/Mapbox/node-mapnik/build/1.0.1567

image

@springmeyer springmeyer removed this from the v3.5.14 milestone Sep 9, 2016
@wilhelmberg
Copy link
Contributor

Mixing compiler/runtime versions is generally a bad thing and should be avoided (*).
That's why we also build all dependencies from scratch.

To be investigated:

  • why node-mapnik (/MD) compiles successfully against stock node.lib (/MT) without mismatch detected for 'RuntimeLibrary' error.
  • why all node-mapnik tests pass when built against stock node.lib (/MT) as seen above.

Next steps:

  • shortcut: create a "stress test" to check if something blows up using stock /MT node
  • long route: Find out if/how CRT objects are passed across module boundaries
    • if yes, then this is a no-go and we have to keep building our own /MD node
    • if no, then we can move on with stock /MT node

(*)
except all code that uses CRT objects and is designed in a way that it does not pass these objects across module boundaries.


Additional reads about compiler versions and mixing /MD with /MT:

@springmeyer
Copy link
Member Author

next action here is to modify the node-cpp11 builds to use the right directory structure for libs and headers that new node-gyp expects. That is the core problem. That was manually fixed by @BergWerkGIS for node v4/5. We should return to this in 2017 and fix node-cpp11 to build the right structure for >= v4 and up automatically. @BergWerkGIS let's hit this in 2017.

@springmeyer
Copy link
Member Author

Update: got a branch that confirms what we've seen above: moving back to stock node.exe works (as far as builds and tests) when the --node-shared=true flag is passed: #758

Next action is to followup on @BergWerkGIS's next steps above:

  • shortcut: create a "stress test" to check if something blows up using stock /MT node
  • long route: Find out if/how CRT objects are passed across module boundaries
    • if yes, then this is a no-go and we have to keep building our own /MD node
    • if no, then we can move on with stock /MT node

I vote for the shortcut first :) @BergWerkGIS let's sync up in the morning on this.

@wilhelmberg
Copy link
Contributor

wilhelmberg commented Apr 20, 2017

hm, hitting a wall with [email protected] and [email protected] locally:

λ npm install [email protected]
npm WARN package.json [email protected] No license field.
-
> [email protected] install c:\mb\investigate-gpx\node_modules\mapnik
> node-pre-gyp install --fallback-to-build

node-pre-gyp ERR! Tried to download(undefined): https://mapbox-node-binary.s3.amazonaws.com/mapnik/v3.6.0/node-v46-win32-x64-Release.tar.gz
node-pre-gyp ERR! Pre-built binaries not found for [email protected] and [email protected] (node-v46 ABI) (falling back to source compile with node-gyp)

But binary package (https://mapbox-node-binary.s3.amazonaws.com/mapnik/v3.6.0/node-v46-win32-x64-Release.tar.gz) does exist.
???

@wilhelmberg
Copy link
Contributor

BTW, same with gdal:

λ npm install gdal
npm WARN package.json [email protected] No license field.
-
> [email protected] preinstall c:\mb\investigate-gpx\node_modules\gdal
> npm install node-pre-gyp

npm WARN engineStrict Per-package engineStrict (found in this package's package.json)
npm WARN engineStrict won't be used in npm 3+. Use the config setting `engine-strict` instead.
[email protected] node_modules\node-pre-gyp
├── [email protected]
├── [email protected] ([email protected], [email protected])
├── [email protected] ([email protected], [email protected], [email protected], [email protected])
├── [email protected] ([email protected])
├── [email protected] ([email protected], [email protected], [email protected], [email protected], [email protected], [email protected])
├── [email protected] ([email protected])
├── [email protected] ([email protected], [email protected], [email protected], [email protected])
├── [email protected] ([email protected], [email protected], [email protected])
└── [email protected] ([email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected])

> [email protected] install c:\mb\investigate-gpx\node_modules\gdal
> node-pre-gyp install --fallback-to-build

node-pre-gyp ERR! Tried to download(undefined): https://mapbox-node-binary.s3.amazonaws.com/gdal/v0.9.4/node-v46-win32-x64.tar.gz
node-pre-gyp ERR! Pre-built binaries not found for [email protected] and [email protected] (node-v46 ABI) (falling back to source compile with node-gyp)

@springmeyer
Copy link
Member Author

@BergWerkGIS can you try passing --loglevel=http - that should show more detail about what error is happening under the hood. Usually this means there was no response from the server and this path was hit: https://github.com/mapbox/node-pre-gyp/blob/master/lib/install.js#L106-L109

@wilhelmberg
Copy link
Contributor

Finally got node-mapnik-bench running.

On my laptop:

node bin\bench testcases\geojson\us-counties-polygons.geojson v3.6.0 --minzoom=0 --maxzoom=12

results in

{
  "source": "c:\\mb\\node-mapnik-bench\\testcases\\geojson\\us-counties-polygons.geojson",
  "version": "v3.6.0",
  "options": {
    "maxzoom": 12
  },
  "time": {
    "start": 1492707090468,
    "xml": 1492707091320,
    "bridge": 1492707091398,
    "info": 1492707091398,
    "load": 1492707091399,
    "copy": 1492707189108
  },
  "sink": "noop://",
  "memory": {
    "max_rss": "93.54MB",
    "max_heap": "39.49MB",
    "max_heap_total": "57.25MB"
  },
  "tile_count": 312008
}

Memory usage gradually increased to ~70MB for node.exe, then stayed between ~65MB and ~70MB for the whole time.
Chart looks like a straight line:
image

@springmeyer
Copy link
Member Author

Awesome results @BergWerkGIS! What exact version of node.exe was this with?

@wilhelmberg
Copy link
Contributor

[email protected] downloaded from nodejs.org

@springmeyer
Copy link
Member Author

Awesome and surprising @BergWerkGIS. I presume that node.exe links totally different CRT libs (and library names) since it is built with vs 2013. Do you get the same positive result with node v6?

As far as next steps, I'm thinking your results from node-mapnik-bench are all we need to move forward with this plan besides a gut check on what exactly the --node-shared=true does and confirming it feels safe.

@wilhelmberg
Copy link
Contributor

wilhelmberg commented Apr 24, 2017

Finally made node-mapnik-bench work with [email protected] and [email protected] on Windows.
Installed node-mapnik via:
npm install https://github.com/mapnik/node-mapnik/tarball/win-test-dane


/cc @mapsam probably a blue sky task?
Make node-mapnik-bench a bit less user friendly but with as few dependencies as possible?
eg provide a pregenerated XML for the test data so that a call to mapnik-omnivore is not necessary.

Some of the things I had to do to get going (from the top of my head, unfortunately I didn't take notes):

  • manually copy around binary dependencies, eg copy (and sometimes overwrite) mapnik binaries to several places
    • some modules expected them at lib/binding
    • others at lib/binding/node-v48-win32-x64
    • some modules are referencing [email protected]
  • duplicate tilelive-bridge
    • some modules expect it under the @mapbox namspace, some don't
  • reference the github repos directly as latest versions don't seem to be published to npm yet
    tilelive-bridge: no node@6 binaries for referenced mapnik version
    tilelive-omnivore: no node@6 binaries for referenced gdal version

Maybe some of the above problems also stem from [email protected] flattening the dependencies?


Anyway

node bin\bench testcases\geojson\us-counties-polygons.geojson v3.6.0 --minzoom=0 --maxzoom=12

yielded:

{
  "source": "C:\\b\\testcases\\geojson\\us-counties-polygons.geojson",
  "version": "v3.6.0",
  "options": {
    "maxzoom": 12
  },
  "time": {
    "start": 1493024100177,
    "xml": 1493024100958,
    "bridge": 1493024101015,
    "info": 1493024101016,
    "load": 1493024101016,
    "copy": 1493024152762
  },
  "sink": "noop://",
  "memory": {
    "max_rss": "76.71MB",
    "max_heap": "23.94MB",
    "max_heap_total": "44.04MB"
  },
  "tile_count": 312007
}

I didn't notice any weird memory usage related behavior in Task Manager during execution of the above command.

@springmeyer
Copy link
Member Author

As far as next steps, I'm thinking your results from node-mapnik-bench are all we need to move forward with this plan besides a gut check on what exactly the --node-shared=true does and confirming it feels safe.

I dug and found this can be reviewed by following nodejs/node#9385 (comment) to nodejs/node@527db40...e97723b, which is a backport of nodejs/node#7487 (and a few other followups that landed in master)

My understanding is:

  • The --node-shared=true is primarily about building node as a dll correctly by using /MD to link node itself to its deps.
  • We don't care about using node itself as a dll, we only care about linking node-mapnik to libmapnik.dll in a way that allows node-mapnik to work with any node.exe binary (dll/MD or not/MT)
  • In our case - passing --node-shared=true from an addon build when using the stock node.exe we are basically saying:
    • We know that node itself is not built with this flag, but we pass it anyway
    • When passing it we functionally disable the default /MT linking that is inherited from node's gyp and would otherwise impact our addon build
    • So, our addon can be built /MD even though node itself is not

While unconventional, this appears to work great and proves that, at least in the case of node addons like node-mapnik, the boundary crossing issues described at https://www.softwariness.com/articles/visual-cpp-runtime-libraries/ do not apply (nothing like std::string is allocated in node and deallocated by node-mapnik).

@springmeyer
Copy link
Member Author

springmeyer commented May 20, 2017

To recap:

  • This ticket started with work towards building against against node v6 long ago
  • The underlying problem is that for node v0.10, v0.12, and v4 we've used customized builds of node (built with /MD) via https://github.com/mapbox/node-cpp11
  • In the process of further testing we've found that we can use the stock node.exe (built with /MT) and we can avoid patching by using the --node-shared=true flag with node >= v4.

Working on getting this working at #758

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

No branches or pull requests

2 participants