WebUI: Apply local patch to vulcanize.

WebUI: Apply local patch to vulcanize.

The local patch unblocks the GN/Ninja migration for vulcanize.

BUG=673825
Review-Url: https://codereview.chromium.org/2643723005
Cr-Commit-Position: refs/heads/master@{#445233}
Committed: https://chromium.googlesource.com/chromium/src/+/d60fc621f4162b8bcbf666236508fbf53210bb05

[dpapad, 2017-01-19 01:47:36]

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

[commit-bot: I haz the power, 2017-01-19 01:48:16]

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-01-19 01:56:00]

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

[commit-bot: I haz the power, 2017-01-19 01:56:00]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-01-20 22:28:09]

Description was changed from

==========
WebUI: Apply local patch to vulcanize.

BUG=673825
==========

to

==========
WebUI: Apply local patch to vulcanize.

The local patch unblocks the GN/Ninja migration for vulcanize.

BUG=673825
==========

[dpapad, 2017-01-20 22:28:09]

dpapad@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2017-01-20 23:56:46]

lgtm

[dpapad, 2017-01-21 00:43:19]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-01-21 00:43:47]

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-01-21 00:52:42]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1484959399252220, "parent_rev":
"1c4bf390cfc5619a04b3b67fdc9930234e407fea", "commit_rev":
"d60fc621f4162b8bcbf666236508fbf53210bb05"}

[commit-bot: I haz the power, 2017-01-21 00:53:18]

Description was changed from

==========
WebUI: Apply local patch to vulcanize.

The local patch unblocks the GN/Ninja migration for vulcanize.

BUG=673825
==========

to

==========
WebUI: Apply local patch to vulcanize.

The local patch unblocks the GN/Ninja migration for vulcanize.

BUG=673825

Review-Url: https://codereview.chromium.org/2643723005
Cr-Commit-Position: refs/heads/master@{#445233}
Committed:
https://chromium.googlesource.com/chromium/src/+/d60fc621f4162b8bcbf666236508...
==========

[commit-bot: I haz the power, 2017-01-21 00:53:19]

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

[Nico, 2017-01-22 21:50:41]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2017-01-22 21:50:41]

why don't we upstream this instead?

https://codereview.chromium.org/2643723005/diff/1/third_party/node/update_npm...
File third_party/node/update_npm_deps (right):

https://codereview.chromium.org/2643723005/diff/1/third_party/node/update_npm...
third_party/node/update_npm_deps:21: # Apply local patch to vulacanize.
typo volacanize

[dpapad, 2017-01-23 17:54:23]

On 2017/01/22 at 21:50:41, thakis wrote:
> why don't we upstream this instead?

Part of the local patch is in the process of being upstreamed, see
https://github.com/Polymer/polymer-bundler/pull/401. The remaining part is
specific to Chrome's usage of vulcanize and it is going to be a harder sell for
upstreaming.


> 
>
https://codereview.chromium.org/2643723005/diff/1/third_party/node/update_npm...
> File third_party/node/update_npm_deps (right):
> 
>
https://codereview.chromium.org/2643723005/diff/1/third_party/node/update_npm...
> third_party/node/update_npm_deps:21: # Apply local patch to vulacanize.
> typo volacanize
Will fix in follow up, thanks.

[Nico, 2017-01-23 18:38:35]

On 2017/01/23 17:54:23, dpapad wrote:
> On 2017/01/22 at 21:50:41, thakis wrote:
> > why don't we upstream this instead?
> 
> Part of the local patch is in the process of being upstreamed, see
> https://github.com/Polymer/polymer-bundler/pull/401. The remaining part is
> specific to Chrome's usage of vulcanize and it is going to be a harder sell
for
> upstreaming.

Maybe upstream would be amenable to some plugin thing? (I don't know what you're
trying to do, but forking vulcanize roughly immediately after adding a dep on it
seems like a suboptimal start).

[Dan Beam, 2017-01-23 18:48:46]

On 2017/01/23 18:38:35, Nico wrote:
> On 2017/01/23 17:54:23, dpapad wrote:
> > On 2017/01/22 at 21:50:41, thakis wrote:
> > > why don't we upstream this instead?
> > 
> > Part of the local patch is in the process of being upstreamed, see
> > https://github.com/Polymer/polymer-bundler/pull/401. The remaining part is
> > specific to Chrome's usage of vulcanize and it is going to be a harder sell
> for
> > upstreaming.
> 
> Maybe upstream would be amenable to some plugin thing? (I don't know what
you're
> trying to do, but forking vulcanize roughly immediately after adding a dep on
it
> seems like a suboptimal start).

the API exists to easily invoke vulcanize/hydrolysis from a node.js script. 
would you like us to start writing scripts with #!/usr/bin/env node as the
shebang?

[Nico, 2017-01-23 18:59:02]

On 2017/01/23 18:48:46, Dan Beam (slow - converging) wrote:
> On 2017/01/23 18:38:35, Nico wrote:
> > On 2017/01/23 17:54:23, dpapad wrote:
> > > On 2017/01/22 at 21:50:41, thakis wrote:
> > > > why don't we upstream this instead?
> > > 
> > > Part of the local patch is in the process of being upstreamed, see
> > > https://github.com/Polymer/polymer-bundler/pull/401. The remaining part is
> > > specific to Chrome's usage of vulcanize and it is going to be a harder
sell
> > for
> > > upstreaming.
> > 
> > Maybe upstream would be amenable to some plugin thing? (I don't know what
> you're
> > trying to do, but forking vulcanize roughly immediately after adding a dep
on
> it
> > seems like a suboptimal start).
> 
> the API exists to easily invoke vulcanize/hydrolysis from a node.js script. 
> would you like us to start writing scripts with #!/usr/bin/env node as the
> shebang?

I don't know what problem you're trying to solve and how node scripts usually
work or if they usually have shebang lines. (I do know that shebang lines don't
work on Windows.)

As a third_party owner, I do know that we try reasonably hard to not have
downstream diffs if it can be helped (because it makes it a lot harder to update
the third-party dep -- yes, it's scripted, but what if the patch stops applying,
etc. We want through several of these "update dep that wasn't updated in a long
time and where none of the people who added it are still around" exercises, they
aren't super fun).

If you want, you can provide some more background and maybe I'll see some way to
do that that doesn't require permanent downstream diffs. (Temporary downstream
diffs that just apply patches locally that have been sent upstream and that have
a chance of being accepted are A-Ok of course.)

[Dan Beam, 2017-01-23 19:07:34]

On 2017/01/23 18:59:02, Nico wrote:
> On 2017/01/23 18:48:46, Dan Beam (slow - converging) wrote:
> > On 2017/01/23 18:38:35, Nico wrote:
> > > On 2017/01/23 17:54:23, dpapad wrote:
> > > > On 2017/01/22 at 21:50:41, thakis wrote:
> > > > > why don't we upstream this instead?
> > > > 
> > > > Part of the local patch is in the process of being upstreamed, see
> > > > https://github.com/Polymer/polymer-bundler/pull/401. The remaining part
is
> > > > specific to Chrome's usage of vulcanize and it is going to be a harder
> sell
> > > for
> > > > upstreaming.
> > > 
> > > Maybe upstream would be amenable to some plugin thing? (I don't know what
> > you're
> > > trying to do, but forking vulcanize roughly immediately after adding a dep
> on
> > it
> > > seems like a suboptimal start).
> > 
> > the API exists to easily invoke vulcanize/hydrolysis from a node.js script. 
> > would you like us to start writing scripts with #!/usr/bin/env node as the
> > shebang?
> 
> I don't know what problem you're trying to solve and how node scripts usually
> work or if they usually have shebang lines. (I do know that shebang lines
don't
> work on Windows.)
> 
> As a third_party owner, I do know that we try reasonably hard to not have
> downstream diffs if it can be helped (because it makes it a lot harder to
update
> the third-party dep -- yes, it's scripted, but what if the patch stops
applying,
> etc. We want through several of these "update dep that wasn't updated in a
long
> time and where none of the people who added it are still around" exercises,
they
> aren't super fun).
> 
> If you want, you can provide some more background and maybe I'll see some way
to
> do that that doesn't require permanent downstream diffs. (Temporary downstream
> diffs that just apply patches locally that have been sent upstream and that
have
> a chance of being accepted are A-Ok of course.)

when we added a dependency on node.js in chromium, I think it was with the
understanding that we wouldn't step on Python's toes.  We use python in many
places, have good support (linting, modules, python binary bundled with the
check out, presubmits, unit tests, the works).  if we start to write scripts
with node.js to support the node-based tools, I'm OK with that, but I'm worried
that folks will misinterpret that as: I want to accomplish a goal, should I
write a Python script or a Node script to accomplish it?  Having 2 ways may not
be useful in this regard.

That's why I worry about adding scripts to Chromium's src/ that invoke node.

Also, for what it's worth, we will continue working with the Polymer team to get
that upstream PR included.  We've worked with them to land like 50 PRs over the
last year.  It may not land in the exact same form (there may be further review
iteration), but I'm confident we'll get something working that suffices our
needs (because others have had similar requests over the years, we found out in
their issue tracker).

tl;dr - I don't want to step on Python's toes; it's basically a temporary patch.

[Nico, 2017-01-23 19:48:32]

On Mon, Jan 23, 2017 at 2:07 PM, <dbeam@chromium.org> wrote:

> On 2017/01/23 18:59:02, Nico wrote:
> > On 2017/01/23 18:48:46, Dan Beam (slow - converging) wrote:
> > > On 2017/01/23 18:38:35, Nico wrote:
> > > > On 2017/01/23 17:54:23, dpapad wrote:
> > > > > On 2017/01/22 at 21:50:41, thakis wrote:
> > > > > > why don't we upstream this instead?
> > > > >
> > > > > Part of the local patch is in the process of being upstreamed, see
> > > > > https://github.com/Polymer/polymer-bundler/pull/401. The
> remaining part
> is
> > > > > specific to Chrome's usage of vulcanize and it is going to be a
> harder
> > sell
> > > > for
> > > > > upstreaming.
> > > >
> > > > Maybe upstream would be amenable to some plugin thing? (I don't know
> what
> > > you're
> > > > trying to do, but forking vulcanize roughly immediately after adding
> a dep
> > on
> > > it
> > > > seems like a suboptimal start).
> > >
> > > the API exists to easily invoke vulcanize/hydrolysis from a node.js
> script.
> > > would you like us to start writing scripts with #!/usr/bin/env node as
> the
> > > shebang?
> >
> > I don't know what problem you're trying to solve and how node scripts
> usually
> > work or if they usually have shebang lines. (I do know that shebang lines
> don't
> > work on Windows.)
> >
> > As a third_party owner, I do know that we try reasonably hard to not have
> > downstream diffs if it can be helped (because it makes it a lot harder to
> update
> > the third-party dep -- yes, it's scripted, but what if the patch stops
> applying,
> > etc. We want through several of these "update dep that wasn't updated in
> a
> long
> > time and where none of the people who added it are still around"
> exercises,
> they
> > aren't super fun).
> >
> > If you want, you can provide some more background and maybe I'll see
> some way
> to
> > do that that doesn't require permanent downstream diffs. (Temporary
> downstream
> > diffs that just apply patches locally that have been sent upstream and
> that
> have
> > a chance of being accepted are A-Ok of course.)
>
> when we added a dependency on node.js in chromium, I think it was with the
> understanding that we wouldn't step on Python's toes. We use python in many
> places, have good support (linting, modules, python binary bundled with the
> check out, presubmits, unit tests, the works). if we start to write scripts
> with node.js to support the node-based tools, I'm OK with that, but I'm
> worried
> that folks will misinterpret that as: I want to accomplish a goal, should I
> write a Python script or a Node script to accomplish it? Having 2 ways may
> not
> be useful in this regard.
>
> That's why I worry about adding scripts to Chromium's src/ that invoke
> node.
>
> Also, for what it's worth, we will continue working with the Polymer team
> to get
> that upstream PR included. We've worked with them to land like 50 PRs over
> the
> last year. It may not land in the exact same form (there may be further
> review
> iteration), but I'm confident we'll get something working that suffices our
> needs (because others have had similar requests over the years, we found
> out in
> their issue tracker).
>
> tl;dr - I don't want to step on Python's toes; it's basically a temporary
> patch.
>

I didn't understand most of that except "temporary patch", but that sounds
good to me :-)

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.