Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(436)

Issue 2017853002: Add functionality to the install_static library to tokenize strings and compare versions. (Closed)

Created:
4 years, 7 months ago by ananta
Modified:
4 years, 6 months ago
CC:
chromium-reviews, bruening+watch_chromium.org, glider+watch_chromium.org, caitkp+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add functionality to the install_static library to tokenize strings and compare versions. This is required for the ongoing work to add crashpad integration in chrome_elf and remove the crashpad integration from chrome.exe As chrome_elf loads very early in the process, we cannot depend on functionality from base which pulls in additional dependencies from user32, etc. I also added a target called install_static_unittests which contains tests for the new functionality in install_static. The MatchPattern test has been moved from chrome_elf to this executable. The utf8_to_utf16 and utf16_to_utf8 functions in install_static are now public functions as they are needed by crashpad. The other changes in this patch are for getting the install_static executable to run on the windows bots. BUG=604923 TBR=dpranke Committed: https://crrev.com/f2e54a999d4bd88062e27c61f995bac80cc025b1 Cr-Commit-Position: refs/heads/master@{#396612}

Patch Set 1 #

Patch Set 2 : Fix build.gn and add validation for version strings #

Patch Set 3 : Fix windows gn builder redness #

Total comments: 54

Patch Set 4 : Address review comments #

Patch Set 5 : Update comment #

Total comments: 2

Patch Set 6 : Address comments #

Total comments: 2

Patch Set 7 : Address comments and fix chrome_install_static.gypi #

Patch Set 8 : Add install_static_unittests to other windows bots #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+435 lines, -70 lines) Patch
M BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M build/all.gyp View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M build/gn_migration.gypi View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/chrome_installer_static.gypi View 1 2 3 4 5 6 1 chunk +34 lines, -0 lines 0 comments Download
M chrome/install_static/BUILD.gn View 1 2 3 1 chunk +16 lines, -0 lines 2 comments Download
A chrome/install_static/install_static_unittests.isolate View 1 2 3 1 chunk +17 lines, -0 lines 0 comments Download
M chrome/install_static/install_util.h View 1 2 3 4 5 6 2 chunks +25 lines, -0 lines 0 comments Download
M chrome/install_static/install_util.cc View 1 2 3 4 5 6 11 chunks +159 lines, -51 lines 0 comments Download
A chrome/install_static/install_util_unittest.cc View 1 2 3 4 1 chunk +120 lines, -0 lines 0 comments Download
M chrome_elf/chrome_elf_util_unittest.cc View 1 chunk +0 lines, -19 lines 0 comments Download
M testing/buildbot/chromium.fyi.json View 1 chunk +6 lines, -0 lines 0 comments Download
M testing/buildbot/chromium.memory.fyi.json View 5 chunks +11 lines, -0 lines 0 comments Download
M testing/buildbot/chromium.win.json View 1 2 3 4 5 6 7 5 chunks +30 lines, -0 lines 0 comments Download
M testing/buildbot/chromium_trybot.json View 1 chunk +6 lines, -0 lines 1 comment Download
M testing/buildbot/gn_isolate_map.pyl View 1 chunk +4 lines, -0 lines 0 comments Download
M tools/valgrind/chrome_tests.py View 2 chunks +4 lines, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 24 (9 generated)
ananta
dpranke for the buildbot stuff. Please check if the list of changes for adding the ...
4 years, 7 months ago (2016-05-27 02:21:02 UTC) #4
grt (UTC plus 2)
round 1! https://codereview.chromium.org/2017853002/diff/40001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2017853002/diff/40001/BUILD.gn#newcode578 BUILD.gn:578: "//chrome/install_static:install_static_unittests", i think this should be in ...
4 years, 6 months ago (2016-05-27 14:13:21 UTC) #5
scottmg
https://codereview.chromium.org/2017853002/diff/40001/chrome/install_static/install_static_unittests.cc File chrome/install_static/install_static_unittests.cc (right): https://codereview.chromium.org/2017853002/diff/40001/chrome/install_static/install_static_unittests.cc#newcode97 chrome/install_static/install_static_unittests.cc:97: } maybe a test for sub-numbers that have leading ...
4 years, 6 months ago (2016-05-27 17:27:23 UTC) #6
ananta
https://codereview.chromium.org/2017853002/diff/40001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2017853002/diff/40001/BUILD.gn#newcode578 BUILD.gn:578: "//chrome/install_static:install_static_unittests", On 2016/05/27 14:13:20, grt (slow) wrote: > i ...
4 years, 6 months ago (2016-05-27 20:05:26 UTC) #7
grt (UTC plus 2)
https://codereview.chromium.org/2017853002/diff/40001/chrome/chrome_installer_static.gypi File chrome/chrome_installer_static.gypi (right): https://codereview.chromium.org/2017853002/diff/40001/chrome/chrome_installer_static.gypi#newcode50 chrome/chrome_installer_static.gypi:50: 'target_name': 'install_static_unittests', On 2016/05/27 20:05:25, ananta wrote: > On ...
4 years, 6 months ago (2016-05-27 20:18:04 UTC) #8
ananta
https://codereview.chromium.org/2017853002/diff/40001/chrome/install_static/install_util.h File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2017853002/diff/40001/chrome/install_static/install_util.h#newcode12 chrome/install_static/install_util.h:12: #include <vector> On 2016/05/27 20:18:03, grt (slow) wrote: > ...
4 years, 6 months ago (2016-05-27 20:42:13 UTC) #9
grt (UTC plus 2)
lgtm https://codereview.chromium.org/2017853002/diff/40001/chrome/install_static/install_util.h File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2017853002/diff/40001/chrome/install_static/install_util.h#newcode12 chrome/install_static/install_util.h:12: #include <vector> On 2016/05/27 20:42:13, ananta wrote: > ...
4 years, 6 months ago (2016-05-27 20:47:35 UTC) #10
scottmg
lgtm https://codereview.chromium.org/2017853002/diff/100001/chrome/chrome_installer_static.gypi File chrome/chrome_installer_static.gypi (right): https://codereview.chromium.org/2017853002/diff/100001/chrome/chrome_installer_static.gypi#newcode80 chrome/chrome_installer_static.gypi:80: 'install_static/install_static_unittests.isolate' tabs here (rietveld's display of them is ...
4 years, 6 months ago (2016-05-27 20:57:59 UTC) #11
ananta
https://codereview.chromium.org/2017853002/diff/40001/chrome/install_static/install_util.h File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2017853002/diff/40001/chrome/install_static/install_util.h#newcode12 chrome/install_static/install_util.h:12: #include <vector> On 2016/05/27 20:47:35, grt (slow) wrote: > ...
4 years, 6 months ago (2016-05-27 21:38:54 UTC) #12
ananta
TBR'ing dpranke. Will address comments in a followup
4 years, 6 months ago (2016-05-28 00:22:09 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2017853002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2017853002/140001
4 years, 6 months ago (2016-05-28 00:35:00 UTC) #17
commit-bot: I haz the power
Committed patchset #8 (id:140001)
4 years, 6 months ago (2016-05-28 00:39:29 UTC) #19
commit-bot: I haz the power
Patchset 8 (id:??) landed as https://crrev.com/f2e54a999d4bd88062e27c61f995bac80cc025b1 Cr-Commit-Position: refs/heads/master@{#396612}
4 years, 6 months ago (2016-05-28 00:41:09 UTC) #21
Nico
this doesn't build in gyp builds: [29574/34047] RULE install_static_unittests_run: isolate_505e5a847558b720a0ef83d1cbafd202 install_static\install_static_unittests.isolate FAILED: install_static_unittests.isolated.gen.json C:\b\depot_tools\python276_bin\python.exe gyp-win-tool ...
4 years, 6 months ago (2016-05-31 14:00:11 UTC) #23
Dirk Pranke
4 years, 6 months ago (2016-05-31 22:11:55 UTC) #24
Message was sent while issue was closed.
https://codereview.chromium.org/2017853002/diff/140001/chrome/install_static/...
File chrome/install_static/BUILD.gn (right):

https://codereview.chromium.org/2017853002/diff/140001/chrome/install_static/...
chrome/install_static/BUILD.gn:34: output_name = "install_static_unittests"
output_name isn't needed here.

https://codereview.chromium.org/2017853002/diff/140001/chrome/install_static/...
chrome/install_static/BUILD.gn:38: include_dirs = [ "$target_gen_dir" ]
this shouldn't be needed, either.

https://codereview.chromium.org/2017853002/diff/140001/testing/buildbot/chrom...
File testing/buildbot/chromium_trybot.json (right):

https://codereview.chromium.org/2017853002/diff/140001/testing/buildbot/chrom...
testing/buildbot/chromium_trybot.json:214: "platforms": [
this file isn't actually used for anything and this change isn't needed, either.

Powered by Google App Engine
This is Rietveld 408576698