Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2830343002:
Add PRESUBMIT script to enforce WebAPK shell apk version update (Closed)
DescriptionAdd PRESUBMIT script to enforce WebAPK shell apk version update
This CL adds PRESUBMIT script to chrome/android/webapk/shell_apk so
that any time code in chrome/android/webapk/shell_apk/ changes,
chrome/android/webapk/shell_apk/shell_apk_version.gni must be updated
accordingly. See comments in presubmit.py for detailed rules.
BUG=708053
Review-Url: https://codereview.chromium.org/2830343002
Cr-Commit-Position: refs/heads/master@{#467779}
Committed: https://chromium.googlesource.com/chromium/src/+/da877aa6097d48a901ef697e62f9796089c57d3a
Patch Set 1 : Add PRESUBMIT script to enforce WebAPK shell apk version update #
Total comments: 4
Patch Set 2 : New rules :) #
Total comments: 12
Patch Set 3 : nit #Patch Set 4 : addressing comments #Patch Set 5 : nit #
Total comments: 1
Patch Set 6 : nit #Messages
Total messages: 27 (15 generated)
Description was changed from ========== Add PRESUBMIT script to enforce shell apk version update BUG=708053 ========== to ========== Add PRESUBMIT script to enforce WebAPK shell apk version update BUG=708053 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add PRESUBMIT script to enforce WebAPK shell apk version update BUG=708053 ========== to ========== Add PRESUBMIT script to enforce WebAPK shell apk version update This CL adds PRESUBMIT script to chrome/android/webapk/shell_apk so that any time code in chrome/android/webapk/shell_apk/src or chrome/android/webapk/AndroidManifest.xml changes, chrome/android/webapk/shell_apk/shell_apk_version.gni must be updated accordingly. BUG=708053 ==========
zpeng@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, PTAL. Thanks!
pkotwicz@chromium.org changed reviewers: + hartmanng@chromium.org
+Glenn for his thoughts https://codereview.chromium.org/2830343002/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/PRESUBMIT.py (right): https://codereview.chromium.org/2830343002/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:31: if f.startswith(SRC_LOCAL_PATH): We should also check whether any files in res/ were updated as well https://codereview.chromium.org/2830343002/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:43: SHELL_APK_VERSION_LOCAL_PATH, items=problems)] This is not quite right. The correct sequence of events: 1) Changes in shell_apk/ are landed 2) ShellAPK is updated on WAM server (hartmanng@'s magical script) 3) CL is landed which modifies shell_apk_version.gni It would be nice if hartmanng@'s magical script submits a Chromium CL which increments the version This means: - We should have a presubmit check which ensures that if shell_apk_version.gni is modified that it is the only file in the CL - We should change the text in the presubmit to reflect that shell_apk_version.gni needs to be changed in a follow up CL How about: "After this CL lands, you will need to 1) Update the WebAPK Minting Server's version of the shell_apk/ code 2) Submit a CL which increments the version in shell_apk_version.gni The version should be incremented *after* updating the code on the WebAPK Minting Server"
https://codereview.chromium.org/2830343002/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/PRESUBMIT.py (right): https://codereview.chromium.org/2830343002/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:43: SHELL_APK_VERSION_LOCAL_PATH, items=problems)] On 2017/04/21 20:17:30, pkotwicz wrote: > This is not quite right. > > The correct sequence of events: > 1) Changes in shell_apk/ are landed > 2) ShellAPK is updated on WAM server (hartmanng@'s magical script) > 3) CL is landed which modifies shell_apk_version.gni We could change things around to make this work, but the server is not currently able to handle that flow. On the official buildbots, we hardcode a shell version number into the templated AndroidManifest.xml file based on what's in shell_apk_version.gni. So if we don't update shell_apk_version.gni, the server will think that nothing has updated. The only thing that the magical script does on the server side is copy files verbatim from the buildbot's storage bucket into the server's backend storage.
Description was changed from ========== Add PRESUBMIT script to enforce WebAPK shell apk version update This CL adds PRESUBMIT script to chrome/android/webapk/shell_apk so that any time code in chrome/android/webapk/shell_apk/src or chrome/android/webapk/AndroidManifest.xml changes, chrome/android/webapk/shell_apk/shell_apk_version.gni must be updated accordingly. BUG=708053 ========== to ========== Add PRESUBMIT script to enforce WebAPK shell apk version update This CL adds PRESUBMIT script to chrome/android/webapk/shell_apk so that any time code in chrome/android/webapk/shell_apk/ changes, chrome/android/webapk/shell_apk/shell_apk_version.gni must be updated accordingly. See comments in presubmit.py for detailed rules. BUG=708053 ==========
Thanks Glenn and Peter, PTAL! https://codereview.chromium.org/2830343002/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/PRESUBMIT.py (right): https://codereview.chromium.org/2830343002/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:31: if f.startswith(SRC_LOCAL_PATH): On 2017/04/21 20:17:30, pkotwicz wrote: > We should also check whether any files in res/ were updated as well Done.
https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/PRESUBMIT.py (right): https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:16: only changed file and changing $CHROME_UPDATE_TIRGGER_VARIABLE should be request: it might be good to go a step further here and ensure that when $CHROME_UPDATE_TIRGGER_VARIABLE is updated, it must match the existing value of $WAM_MINT_TRIGGER_VARIABLE https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:55: wam_mint_trigger_updated_needed = False nit: s/updated_needed/update_needed/ (and elsewhere)
https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/PRESUBMIT.py (right): https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:13: $SHELL_APK_VERSION_LOCAL_PATH should be changed. How about: "If anything in shell_apk/ directory has changed (excluding test files) $WAM_MINT_TRIGGER_VARIABLE should be changed" I think my version is more readable. https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:30: problems = [] This variable is unused https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:32: chrome_update_trigger_is_updated = False This variable is unused https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:38: for line_num, line in f.ChangedContents(): Can you move iterating through the changed contents to a new function? This removes some code which is duplicated in _CheckChromeUpdateTriggerRule() and _CheckWamMintTriggerRule() Perhaps: def _DoesChangedContentsContain(changed_contents, search_for):
Thanks Glenn & Peter, PTAL! https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/PRESUBMIT.py (right): https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:16: only changed file and changing $CHROME_UPDATE_TIRGGER_VARIABLE should be On 2017/04/27 15:05:54, hartmanng wrote: > request: it might be good to go a step further here and ensure that when > $CHROME_UPDATE_TIRGGER_VARIABLE is updated, it must match the existing value of > $WAM_MINT_TRIGGER_VARIABLE Presubmit needs to use lots of regexp to do this while I think making sure these two variables match lies within the CL review's responsibility. So I propose letting presubmit only handling formatting stuff. WDUT? https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:55: wam_mint_trigger_updated_needed = False On 2017/04/27 15:05:54, hartmanng wrote: > nit: s/updated_needed/update_needed/ (and elsewhere) Done.
lgtm
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Patchset #5 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Glenn & Peter, PTAL! https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/PRESUBMIT.py (right): https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:13: $SHELL_APK_VERSION_LOCAL_PATH should be changed. On 2017/04/27 15:54:30, pkotwicz wrote: > How about: "If anything in shell_apk/ directory has changed (excluding test > files) $WAM_MINT_TRIGGER_VARIABLE should be changed" > > I think my version is more readable. Done. https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:30: problems = [] On 2017/04/27 15:54:30, pkotwicz wrote: > This variable is unused Done. https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:32: chrome_update_trigger_is_updated = False On 2017/04/27 15:54:30, pkotwicz wrote: > This variable is unused Done. https://codereview.chromium.org/2830343002/diff/40001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/PRESUBMIT.py:38: for line_num, line in f.ChangedContents(): On 2017/04/27 15:54:30, pkotwicz wrote: > Can you move iterating through the changed contents to a new function? This > removes some code which is duplicated in _CheckChromeUpdateTriggerRule() and > _CheckWamMintTriggerRule() > > Perhaps: > > def _DoesChangedContentsContain(changed_contents, search_for): > > Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nit https://codereview.chromium.org/2830343002/diff/120001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/PRESUBMIT.py (right): https://codereview.chromium.org/2830343002/diff/120001/chrome/android/webapk/... chrome/android/webapk/shell_apk/PRESUBMIT.py:28: def _ChangedContentsContain(changed_contents, key): Nit: Please rename this function to _DoChangedContentsContain() This makes it clearer that the function does not perform any work apart from computing the return value
The CQ bit was checked by zpeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, hartmanng@chromium.org Link to the patchset: https://codereview.chromium.org/2830343002/#ps140001 (title: "nit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493323003089570,
"parent_rev": "639987e67249c41567ca9b7b716ff653d68b478c", "commit_rev":
"da877aa6097d48a901ef697e62f9796089c57d3a"}
Message was sent while issue was closed.
Description was changed from ========== Add PRESUBMIT script to enforce WebAPK shell apk version update This CL adds PRESUBMIT script to chrome/android/webapk/shell_apk so that any time code in chrome/android/webapk/shell_apk/ changes, chrome/android/webapk/shell_apk/shell_apk_version.gni must be updated accordingly. See comments in presubmit.py for detailed rules. BUG=708053 ========== to ========== Add PRESUBMIT script to enforce WebAPK shell apk version update This CL adds PRESUBMIT script to chrome/android/webapk/shell_apk so that any time code in chrome/android/webapk/shell_apk/ changes, chrome/android/webapk/shell_apk/shell_apk_version.gni must be updated accordingly. See comments in presubmit.py for detailed rules. BUG=708053 Review-Url: https://codereview.chromium.org/2830343002 Cr-Commit-Position: refs/heads/master@{#467779} Committed: https://chromium.googlesource.com/chromium/src/+/da877aa6097d48a901ef697e62f9... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/da877aa6097d48a901ef697e62f9... |
