Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2136363002:
Fill in testing Dockerfile stub. (Closed)
|
Created:
4 years, 5 months ago by Jess Modified:
4 years, 5 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFill in testing Dockerfile stub.
The current Dockerfile uploaded with the testing package is just a NOOP stub. This change creates a functional stub from the shared base used by test and engine and handles additional filesystem setup steps for testing.
The updated Dockerfile will be used to validate service support.
BUG=616945, 608487
Committed: https://crrev.com/be16cfb643c62ef097979bfc3db752a02a9c25f5
Cr-Commit-Position: refs/heads/master@{#405275}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Comment rewrite. #Patch Set 3 : Comment rewrite for directory creation. #
Total comments: 13
Patch Set 4 : Spelling and wording updates. #Messages
Total messages: 27 (10 generated)
Description was changed from ========== Fill in testing Dockerfile stub. BUG=616945 ========== to ========== Fill in testing Dockerfile stub. The current Dockerfile uploaded with the testing package is just a NOOP stub. This change creates a functional stub from the shared base used by test and engine and handles additional filesystem setup steps for testing. The updated Dockerfile will be used to validate service support. BUG=616945 ==========
jessicag@chromium.org changed reviewers: + maniscalco@chromium.org
sriramsr@chromium.org changed reviewers: + sriramsr@chromium.org
lgtm
https://codereview.chromium.org/2136363002/diff/1/blimp/engine/testing/Docker... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2136363002/diff/1/blimp/engine/testing/Docker... blimp/engine/testing/Dockerfile:1: # Testing filesystem setup. Built from share setup with engine Dockerfile. I'm having a hard time parsing this comment. Can you reword? Maybe something like: "This Dockerfile is used to build an image containing Blimp Engine test and tests for dependencies of Blimp Engine. It is built on a base image (base:latest) that's also used for running the Engine itself." https://codereview.chromium.org/2136363002/diff/1/blimp/engine/testing/Docker... blimp/engine/testing/Dockerfile:4: RUN mkdir -p /out/test/ What's the thinking behind the directory structure? Do you anticipate having other subdirs under /out? Would /test be simpler?
https://codereview.chromium.org/2136363002/diff/1/blimp/engine/testing/Docker... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2136363002/diff/1/blimp/engine/testing/Docker... blimp/engine/testing/Dockerfile:1: # Testing filesystem setup. Built from share setup with engine Dockerfile. On 2016/07/11 22:55:24, maniscalco wrote: > I'm having a hard time parsing this comment. Can you reword? Maybe something > like: > > "This Dockerfile is used to build an image containing Blimp Engine test and > tests for dependencies of Blimp Engine. It is built on a base image > (base:latest) that's also used for running the Engine itself." Updated comment. I am avoiding saying it builds an image because that isn't actually what is used to run tests. It is a service detail, but one that informs what should be included in this Dockerfile. https://codereview.chromium.org/2136363002/diff/1/blimp/engine/testing/Docker... blimp/engine/testing/Dockerfile:4: RUN mkdir -p /out/test/ On 2016/07/11 22:55:24, maniscalco wrote: > What's the thinking behind the directory structure? Do you anticipate having > other subdirs under /out? Would /test be simpler? The test binaries by default assume src is down two directories. Something that can be easily handled by flags in the service, but I wanted to cut down on the gotchas while hooking everything together.
LGTM % one more comment... https://codereview.chromium.org/2136363002/diff/1/blimp/engine/testing/Docker... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2136363002/diff/1/blimp/engine/testing/Docker... blimp/engine/testing/Dockerfile:4: RUN mkdir -p /out/test/ On 2016/07/11 23:15:33, Jess wrote: > On 2016/07/11 22:55:24, maniscalco wrote: > > What's the thinking behind the directory structure? Do you anticipate having > > other subdirs under /out? Would /test be simpler? > > The test binaries by default assume src is down two directories. Something that > can be easily handled by flags in the service, but I wanted to cut down on the > gotchas while hooking everything together. Ah, I see. OK, I suggest adding a comment here to that effect. That ways it's clear that this isn't an arbitrary directory structure.
Description was changed from ========== Fill in testing Dockerfile stub. The current Dockerfile uploaded with the testing package is just a NOOP stub. This change creates a functional stub from the shared base used by test and engine and handles additional filesystem setup steps for testing. The updated Dockerfile will be used to validate service support. BUG=616945 ========== to ========== Fill in testing Dockerfile stub. The current Dockerfile uploaded with the testing package is just a NOOP stub. This change creates a functional stub from the shared base used by test and engine and handles additional filesystem setup steps for testing. The updated Dockerfile will be used to validate service support. BUG=616945, 608487 ==========
jessicag@chromium.org changed reviewers: + wez@chromium.org
jessicag@chromium.org changed required reviewers: + wez@chromium.org
Adding Wez for OWNERS review post service team lgtms. Please delegate as appropriate.
https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:1: # This Dockerfile is used to build a filesystem enviornment containing typo: environment https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:3: # of Blimp Engine. It is built on a base image (base:latest) that is also used nit: wording; suggest "... on the same base image that is used to run the Engine itself." https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:12: # be changed with flags, the directory stucture is set up to minimize typo: structure https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:13: # potential problems with initial integration. I think what you're saying here is "add all the files, without recursing into directories, and do so under a /two/part path to be consistent with test binaries' expectation of the build output path? My understanding was that with isolates, and now with GN, all test dependencies end up under the output directory - "knowing" that the directory two levels up was the source directory doesn't help unless we're also adding files from the source tree to the bundle, surely?
https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:1: # This Dockerfile is used to build a filesystem enviornment containing On 2016/07/12 20:34:03, Wez wrote: > typo: environment Done. https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:3: # of Blimp Engine. It is built on a base image (base:latest) that is also used On 2016/07/12 20:34:03, Wez wrote: > nit: wording; suggest "... on the same base image that is used to run the Engine > itself." Done. https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:12: # be changed with flags, the directory stucture is set up to minimize On 2016/07/12 20:34:03, Wez wrote: > typo: structure Done. https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:13: # potential problems with initial integration. On 2016/07/12 20:34:03, Wez wrote: > I think what you're saying here is "add all the files, without recursing into > directories, and do so under a /two/part path to be consistent with test > binaries' expectation of the build output path? > > My understanding was that with isolates, and now with GN, all test dependencies > end up under the output directory - "knowing" that the directory two levels up > was the source directory doesn't help unless we're also adding files from the > source tree to the bundle, surely? There are data dependencies under the source tree that are added to the bundle, specifically ../../base/test/data/.
https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:13: # potential problems with initial integration. On 2016/07/12 21:40:25, Jess wrote: > On 2016/07/12 20:34:03, Wez wrote: > > I think what you're saying here is "add all the files, without recursing into > > directories, and do so under a /two/part path to be consistent with test > > binaries' expectation of the build output path? > > > > My understanding was that with isolates, and now with GN, all test > dependencies > > end up under the output directory - "knowing" that the directory two levels up > > was the source directory doesn't help unless we're also adding files from the > > source tree to the bundle, surely? > > There are data dependencies under the source tree that are added to the bundle, > specifically ../../base/test/data/. Oh, interesting - so I suppose it would help in this comment to be clear as to where those get added to the bundle, since it's not obvious at-a-glance. e.g. are they part of base:latest?
https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:13: # potential problems with initial integration. On 2016/07/13 00:43:41, Wez wrote: > On 2016/07/12 21:40:25, Jess wrote: > > On 2016/07/12 20:34:03, Wez wrote: > > > I think what you're saying here is "add all the files, without recursing > into > > > directories, and do so under a /two/part path to be consistent with test > > > binaries' expectation of the build output path? > > > > > > My understanding was that with isolates, and now with GN, all test > > dependencies > > > end up under the output directory - "knowing" that the directory two levels > up > > > was the source directory doesn't help unless we're also adding files from > the > > > source tree to the bundle, surely? > > > > There are data dependencies under the source tree that are added to the > bundle, > > specifically ../../base/test/data/. > > Oh, interesting - so I suppose it would help in this comment to be clear as to > where those get added to the bundle, since it's not obvious at-a-glance. e.g. > are they part of base:latest? Jess, correct me if I'm wrong, I believe they're getting pulled in to the tarball that's fed to "docker build" because the dependencies are are properly expressed via GN.
https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:13: # potential problems with initial integration. On 2016/07/13 15:03:23, maniscalco wrote: > On 2016/07/13 00:43:41, Wez wrote: > > On 2016/07/12 21:40:25, Jess wrote: > > > On 2016/07/12 20:34:03, Wez wrote: > > > > I think what you're saying here is "add all the files, without recursing > > into > > > > directories, and do so under a /two/part path to be consistent with test > > > > binaries' expectation of the build output path? > > > > > > > > My understanding was that with isolates, and now with GN, all test > > > dependencies > > > > end up under the output directory - "knowing" that the directory two > levels > > up > > > > was the source directory doesn't help unless we're also adding files from > > the > > > > source tree to the bundle, surely? > > > > > > There are data dependencies under the source tree that are added to the > > bundle, > > > specifically ../../base/test/data/. > > > > Oh, interesting - so I suppose it would help in this comment to be clear as to > > where those get added to the bundle, since it's not obvious at-a-glance. e.g. > > are they part of base:latest? > > Jess, correct me if I'm wrong, I believe they're getting pulled in to the > tarball that's fed to "docker build" because the dependencies are are properly > expressed via GN. Nick's got it.
lgtm https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:13: # potential problems with initial integration. On 2016/07/13 17:06:46, Jess wrote: > On 2016/07/13 15:03:23, maniscalco wrote: > > On 2016/07/13 00:43:41, Wez wrote: > > > On 2016/07/12 21:40:25, Jess wrote: > > > > On 2016/07/12 20:34:03, Wez wrote: > > > > > I think what you're saying here is "add all the files, without recursing > > > into > > > > > directories, and do so under a /two/part path to be consistent with test > > > > > binaries' expectation of the build output path? > > > > > > > > > > My understanding was that with isolates, and now with GN, all test > > > > dependencies > > > > > end up under the output directory - "knowing" that the directory two > > levels > > > up > > > > > was the source directory doesn't help unless we're also adding files > from > > > the > > > > > source tree to the bundle, surely? > > > > > > > > There are data dependencies under the source tree that are added to the > > > bundle, > > > > specifically ../../base/test/data/. > > > > > > Oh, interesting - so I suppose it would help in this comment to be clear as > to > > > where those get added to the bundle, since it's not obvious at-a-glance. > e.g. > > > are they part of base:latest? > > > > Jess, correct me if I'm wrong, I believe they're getting pulled in to the > > tarball that's fed to "docker build" because the dependencies are are properly > > expressed via GN. > > Nick's got it. OK, so the issue is that when we build the tarball for the bundle, we include test-data files _with their full source-directory-relative paths_, then?
https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2136363002/diff/40001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:13: # potential problems with initial integration. On 2016/07/13 18:32:05, Wez wrote: > On 2016/07/13 17:06:46, Jess wrote: > > On 2016/07/13 15:03:23, maniscalco wrote: > > > On 2016/07/13 00:43:41, Wez wrote: > > > > On 2016/07/12 21:40:25, Jess wrote: > > > > > On 2016/07/12 20:34:03, Wez wrote: > > > > > > I think what you're saying here is "add all the files, without > recursing > > > > into > > > > > > directories, and do so under a /two/part path to be consistent with > test > > > > > > binaries' expectation of the build output path? > > > > > > > > > > > > My understanding was that with isolates, and now with GN, all test > > > > > dependencies > > > > > > end up under the output directory - "knowing" that the directory two > > > levels > > > > up > > > > > > was the source directory doesn't help unless we're also adding files > > from > > > > the > > > > > > source tree to the bundle, surely? > > > > > > > > > > There are data dependencies under the source tree that are added to the > > > > bundle, > > > > > specifically ../../base/test/data/. > > > > > > > > Oh, interesting - so I suppose it would help in this comment to be clear > as > > to > > > > where those get added to the bundle, since it's not obvious at-a-glance. > > e.g. > > > > are they part of base:latest? > > > > > > Jess, correct me if I'm wrong, I believe they're getting pulled in to the > > > tarball that's fed to "docker build" because the dependencies are are > properly > > > expressed via GN. > > > > Nick's got it. > > OK, so the issue is that when we build the tarball for the bundle, we include > test-data files _with their full source-directory-relative paths_, then? Yes.
The CQ bit was checked by jessicag@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sriramsr@chromium.org, maniscalco@chromium.org Link to the patchset: https://codereview.chromium.org/2136363002/#ps60001 (title: "Spelling and wording updates.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fill in testing Dockerfile stub. The current Dockerfile uploaded with the testing package is just a NOOP stub. This change creates a functional stub from the shared base used by test and engine and handles additional filesystem setup steps for testing. The updated Dockerfile will be used to validate service support. BUG=616945, 608487 ========== to ========== Fill in testing Dockerfile stub. The current Dockerfile uploaded with the testing package is just a NOOP stub. This change creates a functional stub from the shared base used by test and engine and handles additional filesystem setup steps for testing. The updated Dockerfile will be used to validate service support. BUG=616945, 608487 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fill in testing Dockerfile stub. The current Dockerfile uploaded with the testing package is just a NOOP stub. This change creates a functional stub from the shared base used by test and engine and handles additional filesystem setup steps for testing. The updated Dockerfile will be used to validate service support. BUG=616945, 608487 ========== to ========== Fill in testing Dockerfile stub. The current Dockerfile uploaded with the testing package is just a NOOP stub. This change creates a functional stub from the shared base used by test and engine and handles additional filesystem setup steps for testing. The updated Dockerfile will be used to validate service support. BUG=616945, 608487 Committed: https://crrev.com/be16cfb643c62ef097979bfc3db752a02a9c25f5 Cr-Commit-Position: refs/heads/master@{#405275} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/be16cfb643c62ef097979bfc3db752a02a9c25f5 Cr-Commit-Position: refs/heads/master@{#405275} |
