Adding target to build SwiftShader

Adding target to build SwiftShader

Added a new group in third_party/BUILD.gn in order to enable building SwiftShader using 'ninja -C out/Default swiftshader' or similar command. The 'swiftshader' target already existed in third_party/swiftshader/BUILD.gn, but was unaccessible due to not being referenced in any other BUILD.gn files. Note that SwiftShader does not automatically build with Chromium.

BUG=630728
Committed: https://crrev.com/acd4cfcb54ede93fd2364fbb1c0ba050100b42e5
Cr-Commit-Position: refs/heads/master@{#417017}

[sugoi1, 2016-08-29 21:10:13]

sugoi@chromium.org changed reviewers:
+ kbr@chromium.org, thakis@chromium.org

[sugoi1, 2016-08-29 21:10:13]

Adding Thakis@ to get an owner as reviewer.

[sugoi1, 2016-08-29 21:10:40]

The CQ bit was checked by sugoi@chromium.org to run a CQ dry run

[Nico, 2016-08-29 21:18:11]

Two things:

1. The more common thing would be to add a dep (instead of a new group) to the
root target in //BUILD.gn

2. Since swiftshader isn't part of the build yet: Maybe there's a good reason
for that? Does it take a long time to build? No matter if you hook it up here or
in the root file, it'll end up as part of the 'all' target.

[Ken Russell (switch to Gerrit), 2016-08-29 23:50:06]

I'll have to defer to Nico on this review.

[sugoi, 2016-08-30 00:08:39]

On 2016/08/29 21:18:11, Nico wrote:
> Two things:
> 
> 1. The more common thing would be to add a dep (instead of a new group) to the
> root target in //BUILD.gn
> 
> 2. Since swiftshader isn't part of the build yet: Maybe there's a good reason
> for that? Does it take a long time to build? No matter if you hook it up here
or
> in the root file, it'll end up as part of the 'all' target.

Ah, I was hoping to have SwiftShader only built when requested, but I see that
this is not possible. SwiftShader currently isnt't part of the build because
it's a component of Chromium, not an actual dependency. It can be built
separately and used by setting the --swiftshader-path flag, but it isn't a
mandatory part of Chromium.

I'll change my strategy then. I'll restrict building SwiftShader to
Win/Linux/Mac (as it should be) and I'll make sure no warnings occur on any of
these platforms, on any of these compilers (Win/clang seems to have an issue
right now).

[commit-bot: I haz the power, 2016-08-30 02:20:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 03:43:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 03:43:54]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[sugoi1, 2016-09-01 15:37:11]

The CQ bit was checked by sugoi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 15:37:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 16:34:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 16:34:56]

Dry run: This issue passed the CQ dry run.

[sugoi1, 2016-09-01 17:56:25]

On 2016/08/30 00:08:39, sugoi wrote:
> On 2016/08/29 21:18:11, Nico wrote:
> > Two things:
> > 
> > 1. The more common thing would be to add a dep (instead of a new group) to
the
> > root target in //BUILD.gn
> > 
> > 2. Since swiftshader isn't part of the build yet: Maybe there's a good
reason
> > for that? Does it take a long time to build? No matter if you hook it up
here
> or
> > in the root file, it'll end up as part of the 'all' target.
> 
> Ah, I was hoping to have SwiftShader only built when requested, but I see that
> this is not possible. SwiftShader currently isnt't part of the build because
> it's a component of Chromium, not an actual dependency. It can be built
> separately and used by setting the --swiftshader-path flag, but it isn't a
> mandatory part of Chromium.
> 
> I'll change my strategy then. I'll restrict building SwiftShader to
> Win/Linux/Mac (as it should be) and I'll make sure no warnings occur on any of
> these platforms, on any of these compilers (Win/clang seems to have an issue
> right now).

Hey Nico,

So I chose to leave the swiftshader group there for now, even though it's
uncommon to do things this way. I'm fine with SwiftShader being built when
targeting "all" and not when targeting "chrome". I think most chrome developers
will target "chrome" in their day to day work and I feel this causes less of a
disturbance this way. If you feel strongly about adding it as a chrome
dependency, I will do the change, no problem.
My goal is to have bots build and use SwiftShader, so these bots can explicitly
target "swiftshader" if they require that library.
Oh, and the reason SwiftShader isn't built with chromium yet is that it was just
recently added to the DEPS file, after being open sourced in June.
Tell me what you think.

Thanks

[Nico, 2016-09-01 17:59:13]

On 2016/09/01 17:56:25, sugoi1 wrote:
> On 2016/08/30 00:08:39, sugoi wrote:
> > On 2016/08/29 21:18:11, Nico wrote:
> > > Two things:
> > > 
> > > 1. The more common thing would be to add a dep (instead of a new group) to
> the
> > > root target in //BUILD.gn
> > > 
> > > 2. Since swiftshader isn't part of the build yet: Maybe there's a good
> reason
> > > for that? Does it take a long time to build? No matter if you hook it up
> here
> > or
> > > in the root file, it'll end up as part of the 'all' target.
> > 
> > Ah, I was hoping to have SwiftShader only built when requested, but I see
that
> > this is not possible. SwiftShader currently isnt't part of the build because
> > it's a component of Chromium, not an actual dependency. It can be built
> > separately and used by setting the --swiftshader-path flag, but it isn't a
> > mandatory part of Chromium.
> > 
> > I'll change my strategy then. I'll restrict building SwiftShader to
> > Win/Linux/Mac (as it should be) and I'll make sure no warnings occur on any
of
> > these platforms, on any of these compilers (Win/clang seems to have an issue
> > right now).
> 
> Hey Nico,
> 
> So I chose to leave the swiftshader group there for now, even though it's
> uncommon to do things this way. I'm fine with SwiftShader being built when
> targeting "all" and not when targeting "chrome". I think most chrome
developers
> will target "chrome" in their day to day work and I feel this causes less of a
> disturbance this way. If you feel strongly about adding it as a chrome
> dependency, I will do the change, no problem.

If you add it as a dep to toplevel BUILD.gn, it still won't be a dep of
'chrome', just of 'all'.

> My goal is to have bots build and use SwiftShader, so these bots can
explicitly
> target "swiftshader" if they require that library.
> Oh, and the reason SwiftShader isn't built with chromium yet is that it was
just
> recently added to the DEPS file, after being open sourced in June.
> Tell me what you think.

How long does swiftshader need to compile? We have a bunch of bots that build
'all'.

[sugoi1, 2016-09-01 19:40:11]

On 2016/09/01 17:59:13, Nico wrote:
> On 2016/09/01 17:56:25, sugoi1 wrote:
> > On 2016/08/30 00:08:39, sugoi wrote:
> > > On 2016/08/29 21:18:11, Nico wrote:
> > > > Two things:
> > > > 
> > > > 1. The more common thing would be to add a dep (instead of a new group)
to
> > the
> > > > root target in //BUILD.gn
> > > > 
> > > > 2. Since swiftshader isn't part of the build yet: Maybe there's a good
> > reason
> > > > for that? Does it take a long time to build? No matter if you hook it up
> > here
> > > or
> > > > in the root file, it'll end up as part of the 'all' target.
> > > 
> > > Ah, I was hoping to have SwiftShader only built when requested, but I see
> that
> > > this is not possible. SwiftShader currently isnt't part of the build
because
> > > it's a component of Chromium, not an actual dependency. It can be built
> > > separately and used by setting the --swiftshader-path flag, but it isn't a
> > > mandatory part of Chromium.
> > > 
> > > I'll change my strategy then. I'll restrict building SwiftShader to
> > > Win/Linux/Mac (as it should be) and I'll make sure no warnings occur on
any
> of
> > > these platforms, on any of these compilers (Win/clang seems to have an
issue
> > > right now).
> > 
> > Hey Nico,
> > 
> > So I chose to leave the swiftshader group there for now, even though it's
> > uncommon to do things this way. I'm fine with SwiftShader being built when
> > targeting "all" and not when targeting "chrome". I think most chrome
> developers
> > will target "chrome" in their day to day work and I feel this causes less of
a
> > disturbance this way. If you feel strongly about adding it as a chrome
> > dependency, I will do the change, no problem.
> 
> If you add it as a dep to toplevel BUILD.gn, it still won't be a dep of
> 'chrome', just of 'all'.

So same code, but in the top level file?

> > My goal is to have bots build and use SwiftShader, so these bots can
> explicitly
> > target "swiftshader" if they require that library.
> > Oh, and the reason SwiftShader isn't built with chromium yet is that it was
> just
> > recently added to the DEPS file, after being open sourced in June.
> > Tell me what you think.
> 
> How long does swiftshader need to compile? We have a bunch of bots that build
> 'all'.

It takes about 75 seconds to build SwiftShader from scratch on Linux.

[sugoi1, 2016-09-06 17:19:57]

On 2016/09/01 19:40:11, sugoi1 wrote:
> On 2016/09/01 17:59:13, Nico wrote:
> > On 2016/09/01 17:56:25, sugoi1 wrote:
> > > On 2016/08/30 00:08:39, sugoi wrote:
> > > > On 2016/08/29 21:18:11, Nico wrote:
> > > > > Two things:
> > > > > 
> > > > > 1. The more common thing would be to add a dep (instead of a new
group)
> to
> > > the
> > > > > root target in //BUILD.gn
> > > > > 
> > > > > 2. Since swiftshader isn't part of the build yet: Maybe there's a good
> > > reason
> > > > > for that? Does it take a long time to build? No matter if you hook it
up
> > > here
> > > > or
> > > > > in the root file, it'll end up as part of the 'all' target.
> > > > 
> > > > Ah, I was hoping to have SwiftShader only built when requested, but I
see
> > that
> > > > this is not possible. SwiftShader currently isnt't part of the build
> because
> > > > it's a component of Chromium, not an actual dependency. It can be built
> > > > separately and used by setting the --swiftshader-path flag, but it isn't
a
> > > > mandatory part of Chromium.
> > > > 
> > > > I'll change my strategy then. I'll restrict building SwiftShader to
> > > > Win/Linux/Mac (as it should be) and I'll make sure no warnings occur on
> any
> > of
> > > > these platforms, on any of these compilers (Win/clang seems to have an
> issue
> > > > right now).
> > > 
> > > Hey Nico,
> > > 
> > > So I chose to leave the swiftshader group there for now, even though it's
> > > uncommon to do things this way. I'm fine with SwiftShader being built when
> > > targeting "all" and not when targeting "chrome". I think most chrome
> > developers
> > > will target "chrome" in their day to day work and I feel this causes less
of
> a
> > > disturbance this way. If you feel strongly about adding it as a chrome
> > > dependency, I will do the change, no problem.
> > 
> > If you add it as a dep to toplevel BUILD.gn, it still won't be a dep of
> > 'chrome', just of 'all'.
> 
> So same code, but in the top level file?
> 
> > > My goal is to have bots build and use SwiftShader, so these bots can
> > explicitly
> > > target "swiftshader" if they require that library.
> > > Oh, and the reason SwiftShader isn't built with chromium yet is that it
was
> > just
> > > recently added to the DEPS file, after being open sourced in June.
> > > Tell me what you think.
> > 
> > How long does swiftshader need to compile? We have a bunch of bots that
build
> > 'all'.
> 
> It takes about 75 seconds to build SwiftShader from scratch on Linux.

I moved it to the top level BUILD file. Verified that it doesn't build with the
'chrome' target, but builds using the 'swiftshader' target.

[sugoi1, 2016-09-06 17:29:51]

The CQ bit was checked by sugoi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 17:30:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 18:27:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 18:27:56]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2016-09-07 00:19:07]

Seems fine -- lgtm -- but I'm not an owner.

https://codereview.chromium.org/2296433003/diff/40001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2296433003/diff/40001/BUILD.gn#newcode497
BUILD.gn:497: # FIXME: Remove the '!is_clang' condition once __nop is supported
by clang-cl
Change FIXME to TODO(sugoi), and file a follow-on bug

[sugoi1, 2016-09-07 14:44:26]

https://codereview.chromium.org/2296433003/diff/40001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2296433003/diff/40001/BUILD.gn#newcode497
BUILD.gn:497: # FIXME: Remove the '!is_clang' condition once __nop is supported
by clang-cl
On 2016/09/07 00:19:07, Ken Russell wrote:
> Change FIXME to TODO(sugoi), and file a follow-on bug

Done.

[Ken Russell (switch to Gerrit), 2016-09-07 17:18:56]

Thanks, still LGTM

[Nico, 2016-09-07 17:20:56]

Generally lgtm

Two quick questions before you land:

1. Why only on linux and windows? (This is fine, just want to know why)
2. Are the 75s with goma?

[sugoi1, 2016-09-07 17:30:55]

On 2016/09/07 17:20:56, Nico wrote:
> Generally lgtm
> 
> Two quick questions before you land:
> 
> 1. Why only on linux and windows? (This is fine, just want to know why)

Right now, SwiftShader's Chrome component only ships on Windows and Linux, so
these are the most important platforms currently. I plan on adding Mac soon, but
I want to make sure it works properly first. And I wouldn't want to revert this
change because of Mac, so I'm adding these cautiously.

> 2. Are the 75s with goma?

No, 75s is using 'ninja -C out/Default swiftshader' on a z620, but I didn't use
"use_goma = true".

[Nico, 2016-09-07 17:32:14]

On 2016/09/07 17:30:55, sugoi1 wrote:
> On 2016/09/07 17:20:56, Nico wrote:
> > Generally lgtm
> > 
> > Two quick questions before you land:
> > 
> > 1. Why only on linux and windows? (This is fine, just want to know why)
> 
> Right now, SwiftShader's Chrome component only ships on Windows and Linux, so
> these are the most important platforms currently. I plan on adding Mac soon,
but
> I want to make sure it works properly first. And I wouldn't want to revert
this
> change because of Mac, so I'm adding these cautiously.

Oh, if it already ships but isn't part of the chromium build, how does this
work? Is it a component-downloaded component? How is that component built?
What's the advantage of making swiftshader build in a chromium checkout then?

[sugoi1, 2016-09-07 17:38:58]

On 2016/09/07 17:32:14, Nico wrote:
> On 2016/09/07 17:30:55, sugoi1 wrote:
> > On 2016/09/07 17:20:56, Nico wrote:
> > > Generally lgtm
> > > 
> > > Two quick questions before you land:
> > > 
> > > 1. Why only on linux and windows? (This is fine, just want to know why)
> > 
> > Right now, SwiftShader's Chrome component only ships on Windows and Linux,
so
> > these are the most important platforms currently. I plan on adding Mac soon,
> but
> > I want to make sure it works properly first. And I wouldn't want to revert
> this
> > change because of Mac, so I'm adding these cautiously.
> 
> Oh, if it already ships but isn't part of the chromium build, how does this
> work? Is it a component-downloaded component? How is that component built?
> What's the advantage of making swiftshader build in a chromium checkout then?

Yes, it's a component. We build the binaries locally and manually put them on
Google Cloud Storage and Chrome's component server picks it up automatically
when we add a new version. As for the reason for adding it here:
1) We want to be able to build chromium and swiftshader together on some bots,
like layout tests, to replace MESA. We want to be able to update SwiftShader's
version on the bots without having to update the user facing component.
2) We plan on making SwiftShader much smaller (which is a current ongoing
effort) and be able to ship it with Chrome directly and have SwiftShader no
longer be a component eventually.

[Nico, 2016-09-07 17:40:05]

Ok, this makes sense for 1 alone. I'm curious to see how 2 goes :-)

Anyhoo, lgtm.

[sugoi1, 2016-09-07 17:41:12]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2016-09-07 17:41:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 19:03:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 19:03:23]

Failed to apply the patch.
On branch working_branch
Your branch is up-to-date with 'origin/refs/pending/heads/master'.
nothing to commit, working tree clean

[commit-bot: I haz the power, 2016-09-07 19:04:46]

Description was changed from

==========
Adding target to build SwiftShader

Added a new group in third_party/BUILD.gn in order to enable building
SwiftShader using 'ninja -C out/Default swiftshader' or similar command. The
'swiftshader' target already existed in third_party/swiftshader/BUILD.gn, but
was unaccessible due to not being referenced in any other BUILD.gn files. Note
that SwiftShader does not automatically build with Chromium.

BUG=630728
==========

to

==========
Adding target to build SwiftShader

Added a new group in third_party/BUILD.gn in order to enable building
SwiftShader using 'ninja -C out/Default swiftshader' or similar command. The
'swiftshader' target already existed in third_party/swiftshader/BUILD.gn, but
was unaccessible due to not being referenced in any other BUILD.gn files. Note
that SwiftShader does not automatically build with Chromium.

BUG=630728

Committed: https://crrev.com/acd4cfcb54ede93fd2364fbb1c0ba050100b42e5
Cr-Commit-Position: refs/heads/master@{#417017}
==========

[commit-bot: I haz the power, 2016-09-07 19:04:47]

Patchset 4 (id:??) landed as
https://crrev.com/acd4cfcb54ede93fd2364fbb1c0ba050100b42e5
Cr-Commit-Position: refs/heads/master@{#417017}

[sugoi1, 2016-09-07 21:00:23]

On 2016/09/07 19:04:47, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/acd4cfcb54ede93fd2364fbb1c0ba050100b42e5
> Cr-Commit-Position: refs/heads/master@{#417017}

Windows buildbot failed. Will investigate tomorrow. Created a revert here:
https://codereview.chromium.org/2316353002/