Bug 130148 – [GTK][CMAKE] build-webkit doesn't detect when the build fails

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Carlos Garcia Campos]

This is green in the bot http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/45543/steps/compile-webkit/logs/stdio

PluginViewX11.cpp:(.text+0xb78): undefined reference to `void WTF::derefIfNotNull<_cairo>(_cairo*)'
collect2: error: ld returned 1 exit status
[100%] Built target WebKitPluginProcess

====================================================================
 WebKit is now built (01m:48s). 
 To run GtkLauncher with this newly-built code, use the
 "Tools/Scripts/run-launcher" script.

====================================================================
program finished with exit code 0
elapsedTime=108.171358

[Brendan Long]

This is really annoying, because it's hard to tell where a build failed. I think I tracked this down to OptionsGTK.cmake:

build_command(COMMAND_LINE_TO_BUILD)
file(WRITE
    ${CMAKE_BINARY_DIR}${CMAKE_FILES_DIRECTORY}/build.sh
    "#!/bin/sh\n"
    "${COMMAND_LINE_TO_BUILD} $@"
)

build_command() by default gives you `make -i`, where `-i` is also known as `--ignore-errors`.

See:

http://www.gnu.org/software/make/manual/html_node/Errors.html#Errors
http://www.cmake.org/cmake/help/v3.0/command/build_command.html

I'm not exactly sure how to fix this yet. Maybe just strip "-i" from the COMMAND_LINE_TO_BUILD?

[Martin Robinson]

I think this is by design, so that people looking at EWS and buildbot output can fix all errors in one change. We should try to track down how this is configured via CMake. After we understand this, there are a couple options:

1. Let users override the setting in their environment.
2. Conditionally enable the setting only for bots.

[Brendan Long]

We should probably stop-on-error by default, and then have a --ignore-errors for bots, since I suspect most humans will want to see their errors immediately.

[Brendan Long]

Oh, and the CMake documentation sounds like it always adds this for makefiles:

> The trailing -- -i option is added for Makefile generators.

This may just be poorly documented though.

[Brendan Long]

I looked through CMake's source code since they documentation wasn't very helpful, and when generating the build_command, it has a "ignoreErrors" argument, which is unconditionally set to "true" in all cases where it gets called:

    std::string makecommand = this->Makefile->GetLocalGenerator()
      ->GetGlobalGenerator()->GenerateCMakeBuildCommand(target, configuration,
                                                      "", true);

With the signature:

    std::string cmGlobalGenerator::GenerateCMakeBuildCommand(
      const std::string& target, const std::string& config,
      const std::string& native,
      bool ignoreErrors)

So if we want to remove the "-i", we'll have to do it manually.

This works:

    string(REPLACE "-i" "" COMMAND_LINE_TO_BUILD ${COMMAND_LINE_TO_BUILD})

I'll look at adding some sort of option.

[Brendan Long]

What if we just turn this off, then let people add --makeargs="-i" ?

[Martin Robinson]

(In reply to comment #6)
> What if we just turn this off, then let people add --makeargs="-i" ?

The issue is with the bots.

[Martin Robinson]

(In reply to comment #7)
> (In reply to comment #6)
> > What if we just turn this off, then let people add --makeargs="-i" ?
> 
> The issue is with the bots.

Sorry. My comment wasn't very useful. What I mean to say is that we should make sure the bots don't suddenly become less useful for developers on other platforms.

[Brendan Long]

Yes, I mean: Can we make this not the default, then fix the bots to run build-webkit --makeargs="-i" ?

[Martin Robinson]

(In reply to comment #9)
> Yes, I mean: Can we make this not the default, then fix the bots to run build-webkit --makeargs="-i" ?

I think we should shoot for making this not the default and fixing the bots all in one patch.

[Brendan Long]

(In reply to comment #10)
> (In reply to comment #9)
> > Yes, I mean: Can we make this not the default, then fix the bots to run build-webkit --makeargs="-i" ?
> 
> I think we should shoot for making this not the default and fixing the bots all in one patch.

I have a patch mostly ready, but don't we want to turn this off for bots too, since right now they can't tell if they're failing?

[Martin Robinson]

I guess for the bots we really want to swap -i for -k.

[Brendan Long]

Created attachment 229420 [details]
Patch

[Brendan Long]

Comment on attachment 229420 [details]
Patch

I just realized that the string replace for -i really needs a comment. I'll fix it soon.

[Martin Robinson]

(In reply to comment #14)
> (From update of attachment 229420 [details])
> I just realized that the string replace for -i really needs a comment. I'll fix it soon.

Looks good so far, though the EWS probably also needs a fix.

[Brendan Long]

Created attachment 229424 [details]
Patch

Ok, now there's a nice comment.

[Martin Robinson]

Comment on attachment 229424 [details]
Patch

Looks great, but do you think you could also fix this for the EWS?

[Brendan Long]

(In reply to comment #17)
> (From update of attachment 229424 [details])
> Looks great, but do you think you could also fix this for the EWS?

Do you know where the EWS build scripts are? I had assumed it was the same as the other build bots.

[Martin Robinson]

I think it's in Tools/Scripts/webkitpy/common/config/ports.py

[Brendan Long]

Created attachment 229473 [details]
Patch

How's this? I changed ports.py so the default makeargs still apply, even if MAKEARGS is set. The defaults are listed first so they can be overridden.

[Brendan Long]

Comment on attachment 229473 [details]
Patch

Hm it's trying to add --keep-going to ninja. I'll look into that..

[Brendan Long]

So if the build bots are running ninja, then this won't work anyway. It only happens with GNU make apparently. We can solve part of the problem by adding ninja-build to the dependencies.

[Martin Robinson]

Comment on attachment 229473 [details]
Patch

You probably want to put this into GtkWK2Port and attach it onto the result of the calling the parent's method.

[Brendan Long]

Created attachment 229476 [details]
Patch

Here's a simpler one that just replaces -i with -k, so at least the error code will be correct. I'm going to look into adding ninja-build to install-dependencies. CMake doesn't do this with ninja by default, so if we can auto-install ninja then most peope won't see this unless they want it.

[WebKit Commit Bot]

Comment on attachment 229476 [details]
Patch

Clearing flags on attachment: 229476

Committed r167385: <http://trac.webkit.org/changeset/167385>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.