Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[build] Publish node packages with RelWithDebInfo #9497

Merged
merged 2 commits into from
Jul 14, 2017
Merged

[build] Publish node packages with RelWithDebInfo #9497

merged 2 commits into from
Jul 14, 2017

Conversation

brunoabinader
Copy link
Member

Follow-up to #9154 - Make node release binaries debuggable - after switching to CircleCI.

From @bsudekum original notes:

This change makes Node release binaries a little bit more debuggable. Now when there is a crash in production, we should get more information including line numbers. There is no speed implications here, the only downside is an increase in binary size (currently, release binaries are ~2mb).

/cc @springmeyer @tmpsantos

@brunoabinader brunoabinader mentioned this pull request Jul 13, 2017
Closed
kkaefer
kkaefer reviewed Jul 13, 2017
View reviewed changes
CMakeLists.txt Outdated
@@ -76,7 +76,7 @@ if(APPLE AND CMAKE_CXX_COMPILER_ID MATCHES ".*Clang")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-error=unused-command-line-argument")
endif()
set(CMAKE_CXX_FLAGS_RELEASE "-Os -DNDEBUG")

set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "-O3 -DNDEBUG")
Copy link
Contributor

Choose a reason for hiding this comment

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

There's also a set(CMAKE_CONFIGURATION_TYPES Debug Release) in line 71 that we should add this to.

@brunoabinader
Copy link
Member Author

node-pre-gyp ERR! publish Cannot publish over existing version
node-pre-gyp ERR! publish Update the 'version' field in package.json and try again
node-pre-gyp ERR! publish If the previous version was published in error see:
node-pre-gyp ERR! publish node-pre-gyp unpublish

@bsudekum @springmeyer are you 👍 in bumping up the version from 3.5.4 to 3.5.5?

@brunoabinader brunoabinader mentioned this pull request Jul 13, 2017
Merged
@brunoabinader
Copy link
Member Author

brunoabinader commented Jul 13, 2017 •
edited
Loading

debug/overdraw tests are failing because we need mapbox/mapbox-gl-js#4979 (fixed now)

springmeyer
springmeyer reviewed Jul 13, 2017
View reviewed changes
platform/node/scripts/after_success.sh
@@ -3,10 +3,13 @@
set -e
set -o pipefail

if [[ "${PUBLISH:-}" == "true" ]]; then
if [[ "${BUILDTYPE}" == "Release" ]]; then
if [[ -n ${PUBLISH:-} ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good - but I want to mention that I think it would work just the same without the -n.

springmeyer
springmeyer approved these changes Jul 13, 2017
View reviewed changes
Copy link
Contributor

@springmeyer springmeyer left a comment

Choose a reason for hiding this comment

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

Thanks for getting this landed!

@springmeyer
Copy link
Contributor

Note: I've documented the reason for this approach at https://github.com/mapbox/cpp/blob/master/glossary.md#debuggable-release-build

@springmeyer springmeyer reopened this Jul 13, 2017
@springmeyer
Copy link
Contributor

(sorry did not mean to close)

@brunoabinader brunoabinader merged commit a019f26 into master Jul 14, 2017
@brunoabinader brunoabinader deleted the node-relwithdebinfo branch July 14, 2017 12:28
@springmeyer
Copy link
Contributor

Thanks for landing @brunoabinader - @bsudekum - do you plan/have a timeline for rolling this out? Curious to have confirmation that it helps with backtrace quality for the crashes you've seen in production.

@kkaefer kkaefer added Node.js node-mapbox-gl-native build labels Jul 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kkaefer kkaefer kkaefer left review comments

@springmeyer springmeyer springmeyer approved these changes

@bsudekum bsudekum Awaiting requested review from bsudekum

Assignees
No one assigned
Labels
build Node.js node-mapbox-gl-native
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brunoabinader @springmeyer @kkaefer