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

NDEBUG not defined for Release builds #6887

Closed
jfirebaugh opened this issue Nov 3, 2016 · 7 comments
Closed

NDEBUG not defined for Release builds #6887

jfirebaugh opened this issue Nov 3, 2016 · 7 comments
Labels
release blocker Blocks the next final release
Milestone

Comments

@jfirebaugh
Copy link
Contributor

The stack trace from #6883 indicates that the MBGL_CHECK_ERROR macro was compiled to the error-checking variant in a release build. Most likely, this means that release builds are neglecting to define NDEBUG, and therefore including code intended only for debug builds. This includes GL error checking, debug level logging, and asserts.

@kkaefer, is this something that may have regressed in the switch to cmake?

@jfirebaugh jfirebaugh added the release blocker Blocks the next final release label Nov 3, 2016
@boundsj boundsj added this to the ios-v3.4.0 milestone Nov 3, 2016
@kkaefer
Copy link
Member

kkaefer commented Nov 3, 2016

Nope, Release builds are still built with NDEBUG. However, it seems like f41546e changed the default BUILDTYPE for make ipackage to Debug. Apparently this was in response to 9732f02 defaulting to Debug in the rest of the code as well.

@friedbunny @incanus @1ec5 did you accidentally build iOS packages without BUILDTYPE=Release?

@kkaefer
Copy link
Member

kkaefer commented Nov 3, 2016

We could add something like this:

diff --git a/src/mbgl/gl/debugging.hpp b/src/mbgl/gl/debugging.hpp
index 42c31cc..1f1cf09 100644
--- a/src/mbgl/gl/debugging.hpp
+++ b/src/mbgl/gl/debugging.hpp
@@ -2,6 +2,8 @@

 #ifndef NDEBUG

+#pragma message "WARNING: Building with BUILDTYPE=Debug"
+
 #include <string>

 namespace mbgl {

@mikemorris
Copy link
Contributor

We have a hard fail when trying to publish a Node.js package with BUIILDTYPE=Debug on Travis or Bitrise - would something like this be helpful?

@boundsj
Copy link
Contributor

boundsj commented Nov 3, 2016

@friedbunny @incanus @1ec5 did you accidentally build iOS packages without BUILDTYPE=Release?

@kkaefer we don't use the default when we publish a build. We use a script and it sets the build type to release. See https://github.com/mapbox/mapbox-gl-native/blob/release-ios-v3.4.0/platform/ios/scripts/deploy-packages.sh#L53

@1ec5
Copy link
Contributor

1ec5 commented Nov 4, 2016

One theory is that we’re building the SDK itself in Release, but it’s linking to an mbgl static library built in Debug.

@boundsj
Copy link
Contributor

boundsj commented Nov 4, 2016

When I build locally with SYMBOLS=YES BUILDTYPE=Release make iframework and SYMBOLS=YES make iframework things work as expected in the repro app where there is a zero rect framed map view (as expected means log spew in release and crash in debug). So it does seem like something unexpected happened when I published beta 2. We don't intend to make it possible to publish a debug build using the publish script so I don't think I "ran the script incorrectly" so I will keep digging to figure out what happened.

@boundsj
Copy link
Contributor

boundsj commented Nov 8, 2016

When I built beta.2 I used Xcode 7.3.1 because #6842 (comment). I'm not exactly sure why, but something about xcode-selecting that toolchain (I was on Xcode 8 just before that) seems to have caused this Debug output even though the scripts should always result in Release output.

I spent a bunch of time on this before deciding to turn my computer off and turn it back on it again. This appears to have "fixed" the problem. I'm going to close this and will keep an eye on future releases. I think the solution for this already ticketed: #2844

@boundsj boundsj closed this as completed Nov 8, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release blocker Blocks the next final release
Projects
None yet
Development

No branches or pull requests

5 participants