Re: Add version string in group policy templates (issue 1392383004 by fqj@chromium.org)

0 views
Skip to first unread message

f...@chromium.org

unread,
Oct 26, 2015, 2:51:20 PM10/26/15
to b...@chromium.org, grit-de...@googlegroups.com
Hello ben,
Could you please review this?
Thanks


https://codereview.chromium.org/1392383004/

b...@chromium.org

unread,
Oct 26, 2015, 3:10:01 PM10/26/15
to f...@chromium.org, grit-de...@googlegroups.com
On 2015/10/26 18:51:20, fqj wrote:
> Hello ben,
> Could you please review this?
> Thanks

I don't know what this repo is or who the owner is. Can you find a more
relevant
person to review?

https://codereview.chromium.org/1392383004/

f...@chromium.org

unread,
Oct 26, 2015, 3:14:41 PM10/26/15
to b...@chromium.org, fla...@chromium.org, grit-de...@googlegroups.com
Hello @flackr,
Could you please review this?

Sorry, @ben, cauz you're on Chromite Butler's suggested reviewer list.


https://codereview.chromium.org/1392383004/

fla...@chromium.org

unread,
Oct 30, 2015, 10:43:07 AM10/30/15
to f...@chromium.org, joaod...@chromium.org, grit-de...@googlegroups.com
+joaodasilva I'm not very familiar with this part of grit.

https://codereview.chromium.org/1392383004/

joaod...@chromium.org

unread,
Nov 2, 2015, 9:37:41 AM11/2/15
to f...@chromium.org, fla...@chromium.org, grit-de...@googlegroups.com, atwi...@chromium.org
I can have a look at this one.

@Drew, is there an owner of the policy stuff in grit in the enterprise team?


https://codereview.chromium.org/1392383004/diff/20001/grit/format/policy_templates/writer_configuration.py
File grit/format/policy_templates/writer_configuration.py (right):

https://codereview.chromium.org/1392383004/diff/20001/grit/format/policy_templates/writer_configuration.py#newcode8
grit/format/policy_templates/writer_configuration.py:8:
VERSION_FILE="../chrome/VERSION"
The right way to do this would be to take the Chrome version from gyp
and pass it in the arguments when grit is invoked.

See
https://code.google.com/p/chromium/codesearch#chromium/src/components/policy.gypi&l=467

https://codereview.chromium.org/1392383004/diff/20001/grit/format/policy_templates/writer_configuration.py#newcode61
grit/format/policy_templates/writer_configuration.py:61: version =
_parseVersionFile(VERSION_FILE)
Why isn't the version found in the defines? Shouldn't the invocation of
grit be modified instead to make sure that the version is there?

https://codereview.chromium.org/1392383004/diff/20001/grit/format/policy_templates/writers/android_policy_writer.py
File grit/format/policy_templates/writers/android_policy_writer.py
(right):

https://codereview.chromium.org/1392383004/diff/20001/grit/format/policy_templates/writers/android_policy_writer.py#newcode93
grit/format/policy_templates/writers/android_policy_writer.py:93: +
self._GetChromiumVersionString()
Can you add a test for this

https://codereview.chromium.org/1392383004/

f...@chromium.org

unread,
Nov 2, 2015, 12:07:06 PM11/2/15
to joaod...@chromium.org, fla...@chromium.org, grit-de...@googlegroups.com, atwi...@chromium.org
Hello,

I can update .gypi file to pass path for versionfile, it should be
build/grit_action.gypi rather than policy.gypi you pointed out.
But doing this is a different approach compared to your comment saying
making
version exists in gypi_defines rather than let grit to read version file.

What do you advise to do?

https://codereview.chromium.org/1392383004/

joaod...@chromium.org

unread,
Nov 2, 2015, 12:15:53 PM11/2/15
to f...@chromium.org, fla...@chromium.org, grit-de...@googlegroups.com, atwi...@chromium.org
On 2015/11/02 17:07:06, fqj wrote:
> Hello,

> I can update .gypi file to pass path for versionfile, it should be
> build/grit_action.gypi rather than policy.gypi you pointed out.
> But doing this is a different approach compared to your comment saying
> making
> version exists in gypi_defines rather than let grit to read version file.

> What do you advise to do?

writer_configuration.py is already getting a dictionary with defines from
the
build system. That seems like the natural way to pass in the version
information, which is also known to the build system. Why don't we use that?

Have a look at
https://code.google.com/p/chromium/codesearch#chromium/src/build/util/version.gypi.
Seems like "version_full" is a variable available in gyp files that has the
parsed version. I'd just pass that value in the gyp defines.

https://codereview.chromium.org/1392383004/

f...@chromium.org

unread,
Nov 2, 2015, 12:26:31 PM11/2/15
to joaod...@chromium.org, fla...@chromium.org, grit-de...@googlegroups.com, atwi...@chromium.org
removed file parsing, only keep adding version comment to andriod.

PTAL


https://codereview.chromium.org/1392383004/

joaod...@chromium.org

unread,
Nov 2, 2015, 12:30:44 PM11/2/15
to f...@chromium.org, fla...@chromium.org, grit-de...@googlegroups.com, atwi...@chromium.org

f...@chromium.org

unread,
Nov 9, 2015, 1:30:02 PM11/9/15
to joaod...@chromium.org, fla...@chromium.org, grit-de...@googlegroups.com, atwi...@chromium.org
Hello flackr,
Who can help to commit this?


https://codereview.chromium.org/1392383004/

joaod...@chromium.org

unread,
Nov 10, 2015, 3:36:07 AM11/10/15
to f...@chromium.org, fla...@chromium.org, grit-de...@googlegroups.com, atwi...@chromium.org
On 2015/11/09 18:30:01, fqj wrote:
> Hello flackr,
> Who can help to commit this?

In MUC you can ask markusheintz@ or pastarmovj@. You can also ask mnissler@

https://codereview.chromium.org/1392383004/

mnis...@chromium.org

unread,
Nov 19, 2015, 2:02:21 PM11/19/15
to f...@chromium.org, joaod...@chromium.org, fla...@chromium.org, tha...@chromium.org, grit-de...@googlegroups.com, atwi...@chromium.org
Nico, what's the process for submitting grit CLs these days?

https://codereview.chromium.org/1392383004/

mnis...@chromium.org

unread,
Nov 19, 2015, 2:29:06 PM11/19/15
to f...@chromium.org, joaod...@chromium.org, fla...@chromium.org, tha...@chromium.org, grit-de...@googlegroups.com, atwi...@chromium.org
On 2015/11/19 19:02:20, Mattias Nissler wrote:
> Nico, what's the process for submitting grit CLs these days?

It looks like grit has become part of the main chromium repository (which
matches the plan discussed some time ago). Can you rebase this change to
apply
to chromium/src/tools/grit?

https://codereview.chromium.org/1392383004/

tha...@chromium.org

unread,
Nov 19, 2015, 2:35:43 PM11/19/15
to f...@chromium.org, joaod...@chromium.org, fla...@chromium.org, mnis...@chromium.org, grit-de...@googlegroups.com, atwi...@chromium.org
On 2015/11/19 19:29:05, Mattias Nissler wrote:
> On 2015/11/19 19:02:20, Mattias Nissler wrote:
> > Nico, what's the process for submitting grit CLs these days?

> It looks like grit has become part of the main chromium repository (which
> matches the plan discussed some time ago). Can you rebase this change to
> apply
> to chromium/src/tools/grit?

Right, you just land them like any other chromium CL – no DEPS roll needed
after
landing.

Make sure to manually run the tests though, as grit's tests aren't part of
the
cq yet. (this is http://crbug.com/555695)

https://codereview.chromium.org/1392383004/
Reply all
Reply to author
Forward
0 new messages