Bug 144746

#if defined(DEVELOPMENT_BUILD) g_setenv("WEBKIT_INJECTED_BUNDLE_PATH", WEBKIT_INJECTED_BUNDLE_PATH, FALSE); #endif The problem is that WEBKIT_INJECTED_BUNDLE_PATH is only defined when DEVELOPMENT_BUILD is on. Previously DEVELOPMENT_BUILD was defined only for development builds, but now it is defined to 0 when off (using the new SET_AND_EXPOSE_TO_BUILD macro). Possible fixes: #if DEVELOPMENT_BUILD g_setenv("WEBKIT_INJECTED_BUNDLE_PATH", WEBKIT_INJECTED_BUNDLE_PATH, FALSE); #endif or: #ifdef WEBKIT_INJECTED_BUNDLE_PATH g_setenv("WEBKIT_INJECTED_BUNDLE_PATH", WEBKIT_INJECTED_BUNDLE_PATH, FALSE); #endif The later seems best to me.

We need to fix all other uses of DEVELOPMENT_BUILD as well, since we always check whether it's defined rather than what it is defined to. Might as well take the opportunity to remove it, since it's redundant with DEVELOPER_MODE.

Created attachment 252595 [details] Patch

Patch is speculative since I'm having trouble building locally at the moment (can't install dependencies I'd removed for testing something else, due to some Fedora repository issues); let's see what EWS thinks.

Comment on attachment 252595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252595&action=review Thanks! > Tools/ChangeLog:22 > +2015-05-07 Michael Catanzaro <mcatanzaro@igalia.com> > + > + [GTK] 2.9.1 build broken with ENABLE_MINIBROWSER=ON > + https://bugs.webkit.org/show_bug.cgi?id=144746 > + > + Reviewed by NOBODY (OOPS!). > + > + * MiniBrowser/gtk/main.c: > + (main): > + duplicated entry here.

Created attachment 252596 [details] Patch

Comment on attachment 252595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252595&action=review > Source/WebCore/platform/gtk/GtkUtilities.cpp:58 > -#if defined(DEVELOPMENT_BUILD) > +#if ENABLE(DEVELOPER_MODE) > static CString topLevelPath() How will this work if ENABLE_DEVELOPER_MODE is not defined? I think that build-webkit just defines DEVELOPER_MODE and not ENABLE_DEVELOPER_MODE.

It won't work, good catch.

(In reply to comment #7) > Comment on attachment 252595 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252595&action=review > > > Source/WebCore/platform/gtk/GtkUtilities.cpp:58 > > -#if defined(DEVELOPMENT_BUILD) > > +#if ENABLE(DEVELOPER_MODE) > > static CString topLevelPath() > > How will this work if ENABLE_DEVELOPER_MODE is not defined? I think that > build-webkit just defines DEVELOPER_MODE and not ENABLE_DEVELOPER_MODE. Oops, I assumed it was :-P

Another speculative patch for EWS. To be clear, this one should work because DEVELOPER_MODE is not defined at all when it is off (whereas DEVELOPMENT_BUILD was defined to 0).

Created attachment 252598 [details] Patch

Comment on attachment 252598 [details] Patch why changing all #if defined with #ifdef? why not just s/DEVELOPMENT_BUILD/DEVELOPER_MODE/s Or is it always defined but set to 0 now for non devel builds? then it should be just #if, no?

I'd also like to include in this patch the new use of the broken idiom from my spellchecking patch. I'm happy to update the patch for that.

(In reply to comment #12) > Comment on attachment 252598 [details] > Patch > > why changing all #if defined with #ifdef? why not just > s/DEVELOPMENT_BUILD/DEVELOPER_MODE/s It doesn't matter; I'll switch it back to #if defined() to make the patch easier to read. But we use #ifdef usually. >Or is it always defined but set to 0 > now for non devel builds? then it should be just #if, no? No, that's why this patch works: DEVELOPER_MODE is not always defined, unlike DEVELOPMENT_BUILD.

Created attachment 252608 [details] Patch

Comment on attachment 252608 [details] Patch Clearing flags on attachment: 252608 Committed r183945: <http://trac.webkit.org/changeset/183945>

All reviewed patches have been landed. Closing bug.

This broke the world. DEVELOPER_MODE is not exposed to the build, that's what DEVELOPMENT_BUILD was for.

Re-opened since this is blocked by bug 144789

The actual problem is that r183416 removed the -DDEVELOPMENT_BUILD=1 definition.

Created attachment 252699 [details] Patch

I assumed CMake would pass the DEVELOPER_MODE option through to the compiler, but that was a terrible assumption (how would it tell the difference from a real option?). And trying to trust EWS to sanity-check this patch was also a bad idea, because evidently only clean builds broke. Let's commit Carlos's patch as-is first, since it is the minimal patch that fixes the build. The rest is just a matter of style: we can follow-up with /s/DEVELOPMENT_BUILD/DEVELOPER_MODE later on, use SET_AND_EXPOSE_TO_BUILD to get DEVELOPER_MODE, and replace the #if defined() with plain #if.

(In reply to comment #22) > I assumed CMake would pass the DEVELOPER_MODE option through to the > compiler, but that was a terrible assumption (how would it tell the > difference from a real option?). And trying to trust EWS to sanity-check > this patch was also a bad idea, because evidently only clean builds broke. The build was not broken, but the tests, because the test infrastructure relies on some things that are only done for development builds (like search for binaries in the ui process binary dir). That's what caused all test to fail, it failed to find all secondary processes. > Let's commit Carlos's patch as-is first, since it is the minimal patch that > fixes the build. The rest is just a matter of style: we can follow-up with > /s/DEVELOPMENT_BUILD/DEVELOPER_MODE later on, use SET_AND_EXPOSE_TO_BUILD to > get DEVELOPER_MODE, and replace the #if defined() with plain #if. I don't think we should use SET_AND_EXPOSE_TO_BUILD, because that only adds a define to cmakeconfig.h, and not all sources files include that header, some "external" tools (at least the MiniBrowser) don't include that header (at least not automatically). So for global compile options it's safer to use add_definitions(-DFOO=Bar). Also I don't see why we need to rename it. For me a development build is just another thing done by the developer mode, but the developer mode is more than that, it also enables tests and minibrowser, for example.

OK, I have no strong preference about the rest.

(In reply to comment #23) > I don't think we should use SET_AND_EXPOSE_TO_BUILD, because that only adds > a define to cmakeconfig.h, and not all sources files include that header, > some "external" tools (at least the MiniBrowser) don't include that header > (at least not automatically). So for global compile options it's safer to > use add_definitions(-DFOO=Bar). No. We just removed most of the definitions. If the tools need to include cmakeconfig.h they should. Please don't undo my work.

(In reply to comment #23) > Also I don't see why we need to rename it. For me a development build is > just another thing done by the developer mode, but the developer mode is > more than that, it also enables tests and minibrowser, for example. This is also inaccurate: martin@dillardmcburdle:~/WebKit/Tools/MiniBrowser/gtk$ git grep cmakeconfig.h BrowserDownloadsBar.c:#include "cmakeconfig.h" BrowserSearchBar.c:#include "cmakeconfig.h" BrowserWindow.c:#include "cmakeconfig.h" main.c:#include "cmakeconfig.h"

(In reply to comment #26) > (In reply to comment #23) > > > Also I don't see why we need to rename it. For me a development build is > > just another thing done by the developer mode, but the developer mode is > > more than that, it also enables tests and minibrowser, for example. > > This is also inaccurate: > > martin@dillardmcburdle:~/WebKit/Tools/MiniBrowser/gtk$ git grep cmakeconfig.h > BrowserDownloadsBar.c:#include "cmakeconfig.h" > BrowserSearchBar.c:#include "cmakeconfig.h" > BrowserWindow.c:#include "cmakeconfig.h" > main.c:#include "cmakeconfig.h" I'm rewriting this patch to use the ENABLE macro. I files that don't include the WTF Platform.h file, this will give an error signalling that the cmakeconfig.h file isn't included. There are few places that do include cmakeconfig.h and not the WTF Platform.h file, so they can just use ENABLE_DEVELOPER_MODE.

(In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #23) > > > > > Also I don't see why we need to rename it. For me a development build is > > > just another thing done by the developer mode, but the developer mode is > > > more than that, it also enables tests and minibrowser, for example. > > > > This is also inaccurate: > > > > martin@dillardmcburdle:~/WebKit/Tools/MiniBrowser/gtk$ git grep cmakeconfig.h > > BrowserDownloadsBar.c:#include "cmakeconfig.h" > > BrowserSearchBar.c:#include "cmakeconfig.h" > > BrowserWindow.c:#include "cmakeconfig.h" > > main.c:#include "cmakeconfig.h" > > I'm rewriting this patch to use the ENABLE macro. I files that don't include > the WTF Platform.h file, this will give an error signalling that the > cmakeconfig.h file isn't included. There are few places that do include > cmakeconfig.h and not the WTF Platform.h file, so they can just use > ENABLE_DEVELOPER_MODE. Building and testing this now, so please everyone hang tight. We can solve this issue somehow.

(In reply to comment #26) > (In reply to comment #23) > > > Also I don't see why we need to rename it. For me a development build is > > just another thing done by the developer mode, but the developer mode is > > more than that, it also enables tests and minibrowser, for example. Sorry, I misread your statement. It isn't inaccurate. This is a philosophical point to be sure. I added DEVELOPER_MODE, but I never considered the two to be separate things. If it's okay with you, I think we can just continue down this path. I prefer that the code files reflect the option that is passed to the build system to avoid confusion.

(In reply to comment #29) > (In reply to comment #26) > > (In reply to comment #23) > > > > > Also I don't see why we need to rename it. For me a development build is > > > just another thing done by the developer mode, but the developer mode is > > > more than that, it also enables tests and minibrowser, for example. > > Sorry, I misread your statement. It isn't inaccurate. This is a > philosophical point to be sure. I added DEVELOPER_MODE, but I never > considered the two to be separate things. If it's okay with you, I think we > can just continue down this path. I prefer that the code files reflect the > option that is passed to the build system to avoid confusion. I've always considered developer mode as a build-webkit feature rather than a build option. I'm fine with using ENABLE(DEVELOPER_MODE) in any case. I don't see a reason to r- my patch, though, it's perfectly valid and fixes a regression.

Created attachment 252724 [details] Patch

Comment on attachment 252724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252724&action=review > Source/cmake/OptionsGTK.cmake:79 > -# FIXME: There is no reason these should be different. > -SET_AND_EXPOSE_TO_BUILD(DEVELOPMENT_BUILD ${DEVELOPER_MODE}) > +SET_AND_EXPOSE_TO_BUILD(ENABLE_DEVELOPER_MODE ${DEVELOPER_MODE}) Maybe renaming this to SET_AND_EXPOSE_TO_CONFIG would be less confusing, unless we enforce somehow that all files include cmakeconfig.h > Tools/MiniBrowser/gtk/main.c:259 > -#if defined(DEVELOPMENT_BUILD) > +#if ENABLE_DEVELOPER_MODE Hopefully MB is the only exception, I guess the build will fail anyway if you try to use ENABLE(FOO) in a file not including Platform.h no? What happens in a file doesn't include cmakeconfig.h? wasn't there a way to include config.h from the build system?

(In reply to comment #32) > Comment on attachment 252724 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252724&action=review > > > Source/cmake/OptionsGTK.cmake:79 > > -# FIXME: There is no reason these should be different. > > -SET_AND_EXPOSE_TO_BUILD(DEVELOPMENT_BUILD ${DEVELOPER_MODE}) > > +SET_AND_EXPOSE_TO_BUILD(ENABLE_DEVELOPER_MODE ${DEVELOPER_MODE}) > > Maybe renaming this to SET_AND_EXPOSE_TO_CONFIG would be less confusing, > unless we enforce somehow that all files include cmakeconfig.h How about SET_AND_ADD_TO_CONFIG_HEADER? > > Tools/MiniBrowser/gtk/main.c:259 > > -#if defined(DEVELOPMENT_BUILD) > > +#if ENABLE_DEVELOPER_MODE > > Hopefully MB is the only exception, I guess the build will fail anyway if > you try to use ENABLE(FOO) in a file not including Platform.h no? What > happens in a file doesn't include cmakeconfig.h? wasn't there a way to > include config.h from the build system? That's a great idea actually. We should make cmakeconfig.h a global compiler header. I think that should be possible.

(In reply to comment #33) > How about SET_AND_ADD_TO_CONFIG_HEADER? Sounds good to me. > That's a great idea actually. We should make cmakeconfig.h a global compiler > header. I think that should be possible. See http://stackoverflow.com/questions/3387453/include-header-files-from-command-line-gcc

Comment on attachment 252724 [details] Patch Clearing flags on attachment: 252724 Committed r184010: <http://trac.webkit.org/changeset/184010>

All reviewed patches have been landed. Closing bug.