Bug 144746 – REGRESSION(r183416): [GTK] Checks for DEVELOPMENT_BUILD are all wrong

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

/builddir/build/BUILD/webkitgtk-2.9.1/Tools/MiniBrowser/gtk/main.c: In function 'main':
/builddir/build/BUILD/webkitgtk-2.9.1/Tools/MiniBrowser/gtk/main.c:260:45: error: 'WEBKIT_INJECTED_BUNDLE_PATH' undeclared (first use in this function)
     g_setenv("WEBKIT_INJECTED_BUNDLE_PATH", WEBKIT_INJECTED_BUNDLE_PATH, FALSE);

[Michael Catanzaro]

#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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 252595 [details]
Patch

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

Comment on attachment 252595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252595&action=review

Thanks!

> Tools/ChangeLog:22
> [phone]  Michael Catanzaro  <[e-mail]>
> +
> +        [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.

[Michael Catanzaro]

Created attachment 252596 [details]
Patch

[Martin Robinson]

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.

[Michael Catanzaro]

It won't work, good catch.

[Carlos Garcia Campos]

(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

[Michael Catanzaro]

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).

[Michael Catanzaro]

Created attachment 252598 [details]
Patch

[Carlos Garcia Campos]

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?

[Martin Robinson]

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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

Created attachment 252608 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 252608 [details]
Patch

Clearing flags on attachment: 252608

Committed r183945: <http://trac.webkit.org/changeset/183945>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 144789

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 252699 [details]
Patch

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

(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.

[Michael Catanzaro]

OK, I have no strong preference about the rest.

[Martin Robinson]

(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.

[Martin Robinson]

(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"

[Martin Robinson]

(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.

[Martin Robinson]

(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.

[Martin Robinson]

(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.

[Carlos Garcia Campos]

(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.

[Martin Robinson]

Created attachment 252724 [details]
Patch

[Carlos Garcia Campos]

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?

[Martin Robinson]

(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.

[Michael Catanzaro]

(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

[Martin Robinson]

Comment on attachment 252724 [details]
Patch

Clearing flags on attachment: 252724

Committed r184010: <http://trac.webkit.org/changeset/184010>

[Martin Robinson]

All reviewed patches have been landed.  Closing bug.