Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2764593002:
Getting OS version numbers for Linux. (Closed)
DescriptionGetting OS version numbers for Linux.
Implementing the method to get OS version numbers for Linux and enabled
corresponding unit test.
BUG=NONE
Review-Url: https://codereview.chromium.org/2764593002
Cr-Commit-Position: refs/heads/master@{#458854}
Committed: https://chromium.googlesource.com/chromium/src/+/2cd67b5b3f57bd69c5a88e927852600f7b956912
Patch Set 1 #
Total comments: 3
Patch Set 2 : comments. #Patch Set 3 : Fixing build #Patch Set 4 : Fixing build #
Messages
Total messages: 31 (22 generated)
romax@chromium.org changed reviewers: + dcheng@chromium.org
can you please take a look? referring to this line of TODO: https://cs.chromium.org/chromium/src/base/sys_info.h?rcl=1506f35e2a82f1b50ba2... Thanks!
Description was changed from ========== Getting OS version numbers for Linux. Implementing the method to get OS version numbers for Linux and enabled corresponding unit test. BUG=NONE ========== to ========== Getting OS version numbers for Linux. Implementing the method to get OS version numbers for Linux and enabled corresponding unit test. BUG=NONE ==========
romax@chromium.org changed reviewers: + petewil@chromium.org
https://codereview.chromium.org/2764593002/diff/1/base/sys_info_posix.cc File base/sys_info_posix.cc (right): https://codereview.chromium.org/2764593002/diff/1/base/sys_info_posix.cc#newc... base/sys_info_posix.cc:199: sscanf(info.release, "%d.%d.%d", major_version, minor_version, Is the formatting of |release| guaranteed by some standard?
https://codereview.chromium.org/2764593002/diff/1/base/sys_info_posix.cc File base/sys_info_posix.cc (right): https://codereview.chromium.org/2764593002/diff/1/base/sys_info_posix.cc#newc... base/sys_info_posix.cc:199: sscanf(info.release, "%d.%d.%d", major_version, minor_version, On 2017/03/21 06:51:17, dcheng wrote: > Is the formatting of |release| guaranteed by some standard? Also, I'd probably prefer the approach that iOS uses: https://cs.chromium.org/chromium/src/base/sys_info_ios.mm?rcl=60d92c2f3ff60c2...
thanks for the comments, PTAnL. https://codereview.chromium.org/2764593002/diff/1/base/sys_info_posix.cc File base/sys_info_posix.cc (right): https://codereview.chromium.org/2764593002/diff/1/base/sys_info_posix.cc#newc... base/sys_info_posix.cc:199: sscanf(info.release, "%d.%d.%d", major_version, minor_version, On 2017/03/21 06:53:37, dcheng wrote: > On 2017/03/21 06:51:17, dcheng wrote: > > Is the formatting of |release| guaranteed by some standard? > > Also, I'd probably prefer the approach that iOS uses: > https://cs.chromium.org/chromium/src/base/sys_info_ios.mm?rcl=60d92c2f3ff60c2... Tried a search online and didn't find a standard format for |release|. Also moved the '=0's into the error case calling uname(&info), is it reasonable to have default values here?
lgtm
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2764593002/#ps40001 (title: "Fixing build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for base/sys_info_unittest.cc:
While running git apply --index -p1;
error: patch failed: base/sys_info_unittest.cc:45
error: base/sys_info_unittest.cc: patch does not apply
Patch: base/sys_info_unittest.cc
Index: base/sys_info_unittest.cc
diff --git a/base/sys_info_unittest.cc b/base/sys_info_unittest.cc
index
c3b85077071a8ead6a263947fc109e374516a761..e3592bf28b07a11cb6076ac37c3cccea0c4b546f
100644
--- a/base/sys_info_unittest.cc
+++ b/base/sys_info_unittest.cc
@@ -45,7 +45,7 @@ TEST_F(SysInfoTest, AmountOfTotalDiskSpace) {
<< tmp_path.value();
}
-#if defined(OS_WIN) || defined(OS_MACOSX)
+#if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX)
TEST_F(SysInfoTest, OperatingSystemVersionNumbers) {
int32_t os_major_version = -1;
int32_t os_minor_version = -1;
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2764593002/#ps60001 (title: "Fixing build")
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": 60001, "attempt_start_ts": 1490212346312350,
"parent_rev": "dac09c03b9f3cc44adf71e8836f6bebabec7a61d", "commit_rev":
"2cd67b5b3f57bd69c5a88e927852600f7b956912"}
Message was sent while issue was closed.
Description was changed from ========== Getting OS version numbers for Linux. Implementing the method to get OS version numbers for Linux and enabled corresponding unit test. BUG=NONE ========== to ========== Getting OS version numbers for Linux. Implementing the method to get OS version numbers for Linux and enabled corresponding unit test. BUG=NONE Review-Url: https://codereview.chromium.org/2764593002 Cr-Commit-Position: refs/heads/master@{#458854} Committed: https://chromium.googlesource.com/chromium/src/+/2cd67b5b3f57bd69c5a88e927852... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/2cd67b5b3f57bd69c5a88e927852... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
