Bug 136377 – [Gtk] build.sh needs a -- before make options when the build command is cmake --build

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Brendan Long]

[Gtk] build.sh needs a -- before make options when the build command is cmake --build

[Brendan Long]

Created attachment 237364 [details]
Patch

[Brendan Long]

This is needed on Arch Linux.

[Brendan Long]

Can you take a look at this patch too (or suggest someone else if you don't usually review build system patches)?

[Philippe Normand]

Comment on attachment 237364 [details]
Patch

Why is this patch needed? Do you need to pass specific options to make?

[Martin Robinson]

Comment on attachment 237364 [details]
Patch

This looks like a fix that should be submitted upstream to CMake instead of to WebKit. Why does this only fail for Arch?

[Brendan Long]

(In reply to comment #4)
> (From update of attachment 237364 [details])
> Why is this patch needed? Do you need to pass specific options to make?

webkit-build automatically passes -j8 (or something similar) to the build command.

Specifically, on Arch Linux, build.sh looks like this:

    #!/bin/sh
    /usr/bin/cmake --build . --config "Release" $@

And $@ is -j8.

"cmake --build . --config "Release" -j8" is invalid and gives this error:

    Unknown argument -j8
    Usage: cmake --build <dir> [options] [-- [native-options]]

(In reply to comment #5)
> (From update of attachment 237364 [details])
> This looks like a fix that should be submitted upstream to CMake instead of to WebKit. Why does this only fail for Arch?

The difference between Fedora 20 and Arch seems to be that Arch is using CMake 3.0 and Fedora is using CMake 2.8.

http://www.cmake.org/cmake/help/v3.0/release/3.0.0.html#other-changes

> The build_command() command now returns a cmake(1) --build command line instead of a direct invocation of the native build tool.

This is a WebKit issue because we're calling cmake --build incorrectly. Expecting this to work correctly on two different major versions may be unreasonable, but this was the only change I needed to make to make it work.

For reading the documentation, it looks like sometimes the build command will already include -- though, so I'll update my patch to check for that:

http://www.cmake.org/cmake/help/v3.0/command/build_command.html#command:build_command

[Brendan Long]

Created attachment 237492 [details]
Patch

[Martin Robinson]

Comment on attachment 237492 [details]
Patch

I feel like we tried pretty hard to get the actual build command into the build.sh script. If CMake 3.0 no longer lets us do that, we may need to find another work-around. I cannot recall exactly why it was important that we invoked the build system directly, instead of using "cmake --build."

[Brendan Long]

Fedora 21 is going to ship with CMake 3.0:

https://apps.fedoraproject.org/packages/cmake

The next version of Ubuntu doesn't have it yet, so we might be stuck with both for a little while:

http://packages.ubuntu.com/search?keywords=cmake
https://packages.debian.org/search?keywords=cmake

[Brendan Long]

(In reply to comment #8)
> (From update of attachment 237492 [details])
> I feel like we tried pretty hard to get the actual build command into the build.sh script. If CMake 3.0 no longer lets us do that, we may need to find another work-around. I cannot recall exactly why it was important that we invoked the build system directly, instead of using "cmake --build."

I'm not sure if it's possible to get the real build command anymore..

[Martin Robinson]

(In reply to comment #10)

> I'm not sure if it's possible to get the real build command anymore..

That's really unfortunate. We might have to resort to just creating the command-line manually. :/

[Csaba Osztrogonác]

(In reply to comment #8)
> (From update of attachment 237492 [details])
> I feel like we tried pretty hard to get the actual build command into the build.sh script. If CMake 3.0 no longer lets us do that, we may need to find another work-around. I cannot recall exactly why it was important that we invoked the build system directly, instead of using "cmake --build."

I think you meant bug130076

[Martin Robinson]

(In reply to comment #12)
> (In reply to comment #8)
> > (From update of attachment 237492 [details] [details])
> > I feel like we tried pretty hard to get the actual build command into the build.sh script. If CMake 3.0 no longer lets us do that, we may need to find another work-around. I cannot recall exactly why it was important that we invoked the build system directly, instead of using "cmake --build."
> 
> I think you meant bug130076

Perhaps the original bug is now fixed in the later versions of CMake?

[Martin Robinson]

I think the answer here is that if build.sh is simply calling cmake --build, then we should just not use build.sh for CMake 3.

[Brendan Long]

(In reply to comment #14)
> I think the answer here is that if build.sh is simply calling cmake --build, then we should just not use build.sh for CMake 3.

I'm kind of busy with other things at the moment, so if someone else wants to write a patch to do that it would be fine with me.

[Carlos Garcia Campos]

Created attachment 238648 [details]
Another approach

Martin is right, we don't need the build.sh hack at all with CMake 3.

[Philippe Normand]

Comment on attachment 238648 [details]
Another approach

Nice, thanks!

[Carlos Garcia Campos]

A clean build will be needed, or at least the build.sh script should be removed from the build dir.

[WebKit Commit Bot]

Comment on attachment 238648 [details]
Another approach

Clearing flags on attachment: 238648

Committed r173964: <http://trac.webkit.org/changeset/173964>

[Brendan Long]

Thanks for handling this!