Remove Subzero from recursive DEPS for SwiftShader

Remove Subzero from recursive DEPS for SwiftShader

The SwiftShader repository now contains the Subzero source code using
a Git subtree.

BUG=swiftshader:47
TEST=bots

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2862803003
Cr-Commit-Position: refs/heads/master@{#473912}
Committed: https://chromium.googlesource.com/chromium/src/+/0ba62127e0d5c9a5b4fe25593a0b63dfc10e3359

[capn, 2017-05-04 16:05:17]

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

[commit-bot: I haz the power, 2017-05-04 16:05:41]

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, 2017-05-04 17:06:11]

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

[commit-bot: I haz the power, 2017-05-04 17:06:12]

Dry run: This issue passed the CQ dry run.

[capn, 2017-05-04 18:07:27]

capn@chromium.org changed reviewers:
+ dpranke@chromium.org, jochen@chromium.org

[capn, 2017-05-04 18:07:40]

PTAL

[Dirk Pranke, 2017-05-04 22:33:36]

I'm not sure I understand what's going on here, or whether there's a better
approach to be taken. Let's discuss on the bug.

[Dirk Pranke, 2017-05-04 22:40:12]

dpranke@chromium.org changed reviewers:
+ agable@chromium.org, iannucci@chromium.org

[Dirk Pranke, 2017-05-04 22:40:13]

Actually, let's discuss here instead since I don't have editbugs permissions on
the swiftshader monorail instance.

+iannucci, +agable ...

I don't like the idea of using git subtrees (or git submodules), given that
generally our workflow uses DEPS files (and, in android's case repo manifests)
instead, and most people don't know how to work w/ submodules properly, let
alone subtrees.

I don't understand the comment in the bug about everything needing to be
mirrored into android.googlesource.com vs. the other googlesource.com repos; the
different systems provide the same SLAs and are managed by the same teams on the
same systems, so they're essentially equivalent. For that reason, we allow
android.googlesource repos to be referenced in our DEPS files.

Does mirroring the repo just on android.g.c and DEPSing it in from there solve
that problem? 

Or is there an issue where you *also* don't want to have to keep DEPS and/or the
manifests in sync and updated, and you're hoping to avoid that by using subtree
instead?

[jochen (gone - plz use gerrit), 2017-05-05 14:32:59]

lgtm

[jochen (gone - plz use gerrit), 2017-05-05 14:33:55]

ah, actually seeing dpranke's response now, please wait for that discussion to
end.

for the record, I'd like to avoid recursive DEPS if possible :/

[capn, 2017-05-05 15:09:27]

On 2017/05/04 at 22:40:13, dpranke wrote:
> Actually, let's discuss here instead since I don't have editbugs permissions
on the swiftshader monorail instance.
> 
> +iannucci, +agable ...
> 
> I don't like the idea of using git subtrees (or git submodules), given that
generally our workflow uses DEPS files (and, in android's case repo manifests)
instead, and most people don't know how to work w/ submodules properly, let
alone subtrees.
> 
> I don't understand the comment in the bug about everything needing to be
mirrored into android.googlesource.com vs. the other googlesource.com repos; the
different systems provide the same SLAs and are managed by the same teams on the
same systems, so they're essentially equivalent. For that reason, we allow
android.googlesource repos to be referenced in our DEPS files.
> 
> Does mirroring the repo just on android.g.c and DEPSing it in from there solve
that problem? 
> 
> Or is there an issue where you *also* don't want to have to keep DEPS and/or
the manifests in sync and updated, and you're hoping to avoid that by using
subtree instead?

I believe there are several justifications for using a Git subtree. Subzero is
in maintenance mode, so it's not seeing much development any more, except for my
SwiftShader related changes (15 of the last 20 changes were mine). SwiftShader
is used across Chromium, Android, and google3 projects, as well as external ones
(Firefox etc.), so maintaining multiple approaches to fetch dependencies has
become a burden for our tiny team. The Android team didn't want to depend on
swiftshader.googlesource.com because it's a slippery slope to allow custom
repositories over which they don't have full control (e.g. people could mess up
the access rules right before they go on vacation). Setting up the mirrors and
agreeing on the branching setup and contribution workflows has been a little
painful, but necessary. I'd like to avoid having to go through that again for
Subzero. I don't even know if Android supports recursive dependencies, and
making manifest changes for every Subzero roll, in every branch, seems
cumbersome.

So if we could just regard Subzero as an integral part of SwiftShader, that
would solve it for all projects at once. I was even seriously considering just
dropping in a copy of the current Subzero code and be done with it. Git Subtree
offers the advantage that we still get the history, and it's reasonably
straightforward to move changes upstream or downstream. I acknowledge that this
is a workflow other people may not be familiar with, but so far there have been
zero SwiftShader related changes to Subzero that weren't authored by yours
truly. So the pros far outweigh the cons.

[Dirk Pranke, 2017-05-05 22:46:10]

It is true there are a lot of pros to git subtrees.

However, it does produce some weirdness in the repo, and it can still make for
questions about where
the authoritative source of truth for the (subzero) code is. In addition, much
of our tooling isn't necessarily
set up to deal with the merge commits that subtrees create.

There is a possible alternative, however.

What if we were to merge subzero into swiftshader (either via a subtree or a
roughly equivalent merge
commit that also preserved the history), and then declared that the subzero
directory in swiftshader
was the authoritative source for the code, and then used infra's "gsubtreed"
tooling to create a 
separate mirror of that directory that pnacl could DEPS in? I've talked to
Bradley about it and he
seemed amenable to the idea, but we're checking w/ a couple of others on the
pnacl team. Assuming
they were fine with it, would you be as well?

[capn, 2017-05-08 15:55:38]

capn@chromium.org changed reviewers:
+ stichnot@chromium.org

[capn, 2017-05-08 17:27:29]

On 2017/05/05 at 22:46:10, dpranke wrote:
> It is true there are a lot of pros to git subtrees.
> 
> However, it does produce some weirdness in the repo, and it can still make for
questions about where
> the authoritative source of truth for the (subzero) code is. In addition, much
of our tooling isn't necessarily
> set up to deal with the merge commits that subtrees create.
> 
> There is a possible alternative, however.
> 
> What if we were to merge subzero into swiftshader (either via a subtree or a
roughly equivalent merge
> commit that also preserved the history), and then declared that the subzero
directory in swiftshader
> was the authoritative source for the code, and then used infra's "gsubtreed"
tooling to create a 
> separate mirror of that directory that pnacl could DEPS in? I've talked to
Bradley about it and he
> seemed amenable to the idea, but we're checking w/ a couple of others on the
pnacl team. Assuming
> they were fine with it, would you be as well?

While I agree that we should avoid confusion about the authoritative source for
Subzero, I'm not sure if hosting it as part of SwiftShader is the best solution.
For starters we're only using a subset of Subzero's functionality, so it can
easily regress on the other functionality if we're not careful. Keeping it
firmly rooted in Native Client would help prevent that. I also can't fully
guarantee that we want to keep using Subzero in the long run. And there's no way
of telling if some other project suddenly has a need for something like Subzero
and a substantial number of contributions start flooding in. The subtree
approach is meant to simplify our dependency on Subzero, not increase our
responsibilities. So I'm sorry but I'm not immediately enamored with the idea of
changing the authoritative source.

If Subzero is being abandoned to the point where nobody wants to do reviews for
it any more, then I think the best course of action is for me to fork it into
SwiftShader and rename it, to not have it confused with its Native Client
origins. But forking should be a last resort.

My feeling is that we can keep a middle ground for now and the only complication
is the potential confusion from having two repositories contain the code. But I
don't think it's much of a problem at all. Subzero is under a third_party
directory, and the documentation clarifies its Native Client roots. We could
explicitly mention
https://chromium.googlesource.com/native_client/pnacl-subzero/ there if that
helps. Furthermore, if anyone would attempt to contribute to SwiftShader's copy
of Subzero, I would immediately point them to the upstream repository.

[Dirk Pranke, 2017-05-08 22:37:12]

I think the problem I have is that by inlining it into SwiftShader you're
forking it in all but name, 
and you're doing it in a way that makes things more complicated for people that
are used to 
the way we do things in the other chromium-related repos.

So, to my eyes, this isn't a net win.

[capn, 2017-05-23 14:02:37]

As discussed in our e-mail thread, I've clarified the forking of Subzero in
SwiftShader's documentation (i.e. for now the authoritative source is still
https://chromium.googlesource.com/native_client/pnacl-subzero) and the use of
git-subtree.

[capn, 2017-05-23 14:02:40]

The CQ bit was checked by capn@chromium.org

[commit-bot: I haz the power, 2017-05-23 14:03:17]

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, 2017-05-23 15:45:33]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1495548160762680, "parent_rev":
"b78f1ba01120e4f9dfa9faf541b670d893883cfe", "commit_rev":
"0ba62127e0d5c9a5b4fe25593a0b63dfc10e3359"}

[commit-bot: I haz the power, 2017-05-23 15:45:45]

Description was changed from

==========
Remove Subzero from recursive DEPS for SwiftShader

The SwiftShader repository now contains the Subzero source code using
a Git subtree.

BUG=swiftshader:47

TEST=bots

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
==========

to

==========
Remove Subzero from recursive DEPS for SwiftShader

The SwiftShader repository now contains the Subzero source code using
a Git subtree.

BUG=swiftshader:47

TEST=bots

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2862803003
Cr-Commit-Position: refs/heads/master@{#473912}
Committed:
https://chromium.googlesource.com/chromium/src/+/0ba62127e0d5c9a5b4fe25593a0b...
==========

[commit-bot: I haz the power, 2017-05-23 15:45:47]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/0ba62127e0d5c9a5b4fe25593a0b...

[Dirk Pranke, 2017-05-23 19:38:11]

lgtm