Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2340773002:
Assume ProjectRootPath() equals ../.. in Desktop (Closed)
|
Created:
4 years, 3 months ago by ehmaldonado_webrtc Modified:
4 years, 3 months ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAssume ProjectRootPath() equals ../.. in Desktop
This way we don't have to rely on the existence of DEPS, and the tests
can be run in swarming bots (which don't have a checkout and therefore
don't have a DEPS file).
This seems to be where Chromium is assumming the project root path to
be.
NOTRY=True
BUG=chromium:497757
Committed: https://crrev.com/8b28b8017f31183f24fa2815bd4d3d57b03a8060
Cr-Commit-Position: refs/heads/master@{#14230}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed comments. #
Total comments: 2
Patch Set 3 : Updated comments. #
Messages
Total messages: 25 (12 generated)
ehmaldonado@webrtc.org changed reviewers: + kjellander@chromium.org
Please take a look :)
I executed this with an updated DEPS file in https://codereview.webrtc.org/2334793002 and it made linux and mac swarming bots work.
kjellander@webrtc.org changed reviewers: + kjellander@webrtc.org
Please update the documentation in https://cs.chromium.org/chromium/src/third_party/webrtc/test/testsupport/file... as well. Especially https://cs.chromium.org/chromium/src/third_party/webrtc/test/testsupport/file... https://codereview.webrtc.org/2340773002/diff/1/webrtc/test/testsupport/fileu... File webrtc/test/testsupport/fileutils.cc (right): https://codereview.webrtc.org/2340773002/diff/1/webrtc/test/testsupport/fileu... webrtc/test/testsupport/fileutils.cc:129: // Check for our file that verifies the root dir. I think a comment phrased something like this is suitable here: // Remove two directory levels from the path, i.e. a path like /absolute/path/src/out/Debug/ // will become /absolute/path/src/ https://codereview.webrtc.org/2340773002/diff/1/webrtc/test/testsupport/fileu... webrtc/test/testsupport/fileutils.cc:129: // Check for our file that verifies the root dir. We don't check for a file anymore, do we? I suggest remove it. https://codereview.webrtc.org/2340773002/diff/1/webrtc/test/testsupport/fileu... webrtc/test/testsupport/fileutils.cc:135: // Check for our file that verifies the root dir. Remove this comment, it's just confusing. https://codereview.webrtc.org/2340773002/diff/1/webrtc/test/testsupport/fileu... webrtc/test/testsupport/fileutils.cc:136: size_t path_delimiter_index = path.find_last_of(kPathDelimiter); Does this even compile? It seems you're declaring the same variable a second time. https://codereview.webrtc.org/2340773002/diff/1/webrtc/test/testsupport/fileu... webrtc/test/testsupport/fileutils.cc:138: // Move up one directory in the directory tree. Change to: // Move up another directory level in the directory tree (should now be at the root). https://codereview.webrtc.org/2340773002/diff/1/webrtc/test/testsupport/fileu... webrtc/test/testsupport/fileutils.cc:264: return resources_path + name + "." + extension; return resource_file;
We better run a full CQ set of trybots for this one as well.
Description was changed from ========== Assume ProjectRootPath() equals ../.. in Desktop This way we don't have to rely on the existence of DEPS, and the tests can be run in swarming bots (which don't have a checkout and therefore don't have a DEPS file). This seems to be where Chromium is assumming the project root path to be. BUG=chromium:497757 ========== to ========== Assume ProjectRootPath() equals ../.. in Desktop This way we don't have to rely on the existence of DEPS, and the tests can be run in swarming bots (which don't have a checkout and therefore don't have a DEPS file). This seems to be where Chromium is assumming the project root path to be. BUG=chromium:497757 ==========
kjellander@webrtc.org changed reviewers: - kjellander@chromium.org
Patchset #2 (id:20001) has been deleted
PTAL https://codereview.webrtc.org/2340773002/diff/1/webrtc/test/testsupport/fileu... File webrtc/test/testsupport/fileutils.cc (right): https://codereview.webrtc.org/2340773002/diff/1/webrtc/test/testsupport/fileu... webrtc/test/testsupport/fileutils.cc:129: // Check for our file that verifies the root dir. On 2016/09/15 06:28:32, kjellander_webrtc wrote: > We don't check for a file anymore, do we? I suggest remove it. Done. https://codereview.webrtc.org/2340773002/diff/1/webrtc/test/testsupport/fileu... webrtc/test/testsupport/fileutils.cc:135: // Check for our file that verifies the root dir. On 2016/09/15 06:28:33, kjellander_webrtc wrote: > Remove this comment, it's just confusing. Done. https://codereview.webrtc.org/2340773002/diff/1/webrtc/test/testsupport/fileu... webrtc/test/testsupport/fileutils.cc:136: size_t path_delimiter_index = path.find_last_of(kPathDelimiter); On 2016/09/15 06:28:33, kjellander_webrtc wrote: > Does this even compile? It seems you're declaring the same variable a second > time. Yes, it compiles, and it works. I think maybe it was shadowing the previous declaration. I've removed this line. https://codereview.webrtc.org/2340773002/diff/1/webrtc/test/testsupport/fileu... webrtc/test/testsupport/fileutils.cc:138: // Move up one directory in the directory tree. On 2016/09/15 06:28:32, kjellander_webrtc wrote: > Change to: > // Move up another directory level in the directory tree (should now be at the > root). Done. https://codereview.webrtc.org/2340773002/diff/1/webrtc/test/testsupport/fileu... webrtc/test/testsupport/fileutils.cc:264: return resources_path + name + "." + extension; On 2016/09/15 06:28:33, kjellander_webrtc wrote: > return resource_file; I'm leaving this unchanged. Those two lines were for debugging purposes and I'd forgot to remove them.
lgtm, ready for a full CQ run. https://codereview.webrtc.org/2340773002/diff/1/webrtc/test/testsupport/fileu... File webrtc/test/testsupport/fileutils.cc (right): https://codereview.webrtc.org/2340773002/diff/1/webrtc/test/testsupport/fileu... webrtc/test/testsupport/fileutils.cc:264: return resources_path + name + "." + extension; On 2016/09/15 08:23:15, ehmaldonado_webrtc wrote: > On 2016/09/15 06:28:33, kjellander_webrtc wrote: > > return resource_file; > > I'm leaving this unchanged. Those two lines were for debugging purposes and I'd > forgot to remove them. Acknowledged. https://codereview.webrtc.org/2340773002/diff/40001/webrtc/test/testsupport/f... File webrtc/test/testsupport/fileutils.h (right): https://codereview.webrtc.org/2340773002/diff/40001/webrtc/test/testsupport/f... webrtc/test/testsupport/fileutils.h:80: // For android, it is assumed to be /sdcard/chromium_tests_root/ android -> Android https://codereview.webrtc.org/2340773002/diff/40001/webrtc/test/testsupport/f... webrtc/test/testsupport/fileutils.h:81: // For iOS, this calls IOSOutputPath() in iosfileutils.mm Let's leave out implementation details here. How about: // For iOS, the resource files are assumed to be included in the test's .app bundle.
The CQ bit was checked by ehmaldonado@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2340773002/#ps60001 (title: "Updated comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/11271)
On 2016/09/15 11:07:48, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_dbg on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/11271) Ignore that one, it's flaky: https://codereview.webrtc.org/2341933002/
Description was changed from ========== Assume ProjectRootPath() equals ../.. in Desktop This way we don't have to rely on the existence of DEPS, and the tests can be run in swarming bots (which don't have a checkout and therefore don't have a DEPS file). This seems to be where Chromium is assumming the project root path to be. BUG=chromium:497757 ========== to ========== Assume ProjectRootPath() equals ../.. in Desktop This way we don't have to rely on the existence of DEPS, and the tests can be run in swarming bots (which don't have a checkout and therefore don't have a DEPS file). This seems to be where Chromium is assumming the project root path to be. NOTRY=True BUG=chromium:497757 ==========
On 2016/09/15 11:28:00, kjellander_chromium wrote: > On 2016/09/15 11:07:48, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > win_dbg on master.tryserver.webrtc (JOB_FAILED, > > http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/11271) > > Ignore that one, it's flaky: https://codereview.webrtc.org/2341933002/ Ok, I'll commit.
The CQ bit was checked by ehmaldonado@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Assume ProjectRootPath() equals ../.. in Desktop This way we don't have to rely on the existence of DEPS, and the tests can be run in swarming bots (which don't have a checkout and therefore don't have a DEPS file). This seems to be where Chromium is assumming the project root path to be. NOTRY=True BUG=chromium:497757 ========== to ========== Assume ProjectRootPath() equals ../.. in Desktop This way we don't have to rely on the existence of DEPS, and the tests can be run in swarming bots (which don't have a checkout and therefore don't have a DEPS file). This seems to be where Chromium is assumming the project root path to be. NOTRY=True BUG=chromium:497757 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Assume ProjectRootPath() equals ../.. in Desktop This way we don't have to rely on the existence of DEPS, and the tests can be run in swarming bots (which don't have a checkout and therefore don't have a DEPS file). This seems to be where Chromium is assumming the project root path to be. NOTRY=True BUG=chromium:497757 ========== to ========== Assume ProjectRootPath() equals ../.. in Desktop This way we don't have to rely on the existence of DEPS, and the tests can be run in swarming bots (which don't have a checkout and therefore don't have a DEPS file). This seems to be where Chromium is assumming the project root path to be. NOTRY=True BUG=chromium:497757 Committed: https://crrev.com/8b28b8017f31183f24fa2815bd4d3d57b03a8060 Cr-Commit-Position: refs/heads/master@{#14230} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8b28b8017f31183f24fa2815bd4d3d57b03a8060 Cr-Commit-Position: refs/heads/master@{#14230} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
