|
|
Created:
5 years, 2 months ago by fqj Modified:
5 years ago CC:
grit-developer_googlegroups.com, Andrew T Wilson (Slow) Base URL:
https://chromium.googlesource.com/external/grit-i18n.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd the missing version number comment for AndroidPolicyWriter
BUG=108941
Patch Set 1 : #
Total comments: 3
Patch Set 2 : #
Messages
Total messages: 21 (6 generated)
fqj@chromium.org changed reviewers: + ben@chromium.org
Hello, Could you please help review this? Thanks
Patchset #1 (id:1) has been deleted
Hello ben, Could you please review this? Thanks
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?
fqj@chromium.org changed reviewers: + flackr@chromium.org
Hello @flackr, Could you please review this? Sorry, @ben, cauz you're on Chromite Butler's suggested reviewer list.
flackr@chromium.org changed reviewers: + joaodasilva@chromium.org - ben@chromium.org
+joaodasilva I'm not very familiar with this part of grit.
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_temp... File grit/format/policy_templates/writer_configuration.py (right): https://codereview.chromium.org/1392383004/diff/20001/grit/format/policy_temp... 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.... https://codereview.chromium.org/1392383004/diff/20001/grit/format/policy_temp... 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_temp... File grit/format/policy_templates/writers/android_policy_writer.py (right): https://codereview.chromium.org/1392383004/diff/20001/grit/format/policy_temp... grit/format/policy_templates/writers/android_policy_writer.py:93: + self._GetChromiumVersionString() Can you add a test for this
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?
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.... 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.
Description was changed from ========== Add version string in group policy templates 1. Add the missing version number comment for AndroidPolicyWriter 2. Enable GetConfigurationForBuild to read version number from chrome/VERSION to determine the version number as grit_defines doesn't have information for version number at all. BUG=108941 ========== to ========== Add the missing version number comment for AndroidPolicyWriter BUG=108941 ==========
removed file parsing, only keep adding version comment to andriod. PTAL
lgtm lgtm
Hello flackr, Who can help to commit this?
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@
mnissler@chromium.org changed reviewers: + mnissler@chromium.org, thakis@chromium.org
Nico, what's the process for submitting grit CLs these days?
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?
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) |