Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2154873002:
Update ADD handling in Dockerfiles and test bundle creation (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. |
DescriptionUpdate ADD handling in Dockerfiles and test bundle creation.
Current Dockerfiles were written with a misunderstanding of how wildcards and directory sources interact with the ADD command. This was discovered in testing using the generated tarballs.
The change fixes an incorrect comment in the engine Dockerfile.
Additionally, create_bundle.py was updated to tar up the filesystem state from a context other than the build directory.
These changes mean that for the env_tests bundle, running the Dockerfile in the context of the tarball results in a system file structure such that /blimp/ contains the set of dpendencies for the testing binaries in the same directory structure as // (the chromium src directory).
This change does not remove the duplicate ./third_party/blimp_fonts (./fonts is used instead) for the engine environment.
BUG=616945, 608487, 630438
Committed: https://crrev.com/6df4245122709b6a19e5f898d49a777b153c9bd7
Cr-Commit-Position: refs/heads/master@{#407237}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Move filesystem manipulations for tests into the bundle creation script. #Patch Set 3 : Lint clean up. #Patch Set 4 : Lint clean up. #
Total comments: 4
Patch Set 5 : Rename varables to py style guide. #
Total comments: 12
Patch Set 6 : Dockerfile comments and follow up based on bundle script changes. #Patch Set 7 : Clarifying new script behavior in comments and argument naming. #
Total comments: 12
Patch Set 8 : Minor cleanups. #Patch Set 9 : Remove always true if. #
Total comments: 4
Patch Set 10 : Update create-bundle.py comments to reflect most recent behavior changes. #
Messages
Total messages: 59 (36 generated)
Description was changed from ========== Fix ADD handling in Dockerfiles and associated comments. BUG=616945, 608487 ========== to ========== Fix ADD handling in Dockerfiles and associated comments. Current Dockerfiles were written with a misunderstanding of how wildcards and directory sources interact with the ADD command. This was discovered during testing of the testing export. The change fixes an incorrect comment in the engine Dockerfile and updates the testing Dockerfile to construct the filesystem as required for testing. This change does not remove the duplicate ./third_party/blimp_fonts (./fonts is used instead) for the engine environment. BUG=616945, 608487 ==========
jessicag@chromium.org changed reviewers: + maniscalco@chromium.org, sriramsr@chromium.org, wez@chromium.org
jessicag@chromium.org changed required reviewers: + wez@chromium.org
Clarifying note inline. https://codereview.chromium.org/2154873002/diff/1/blimp/engine/testing/Docker... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2154873002/diff/1/blimp/engine/testing/Docker... blimp/engine/testing/Dockerfile:13: # Docker copies directory CONTENTS rather than the directories themselves. Note for reviewers. This is the point of confusion that existed previously for the Dockerfiles. Documentation https://docs.docker.com/engine/reference/builder/#/add Each <src> may contain wildcards and matching will be done using Go’s filepath.Match rules. ... If <src> is a directory, the entire contents of the directory are copied, including filesystem metadata. Note: The directory itself is not copied, just its contents.
https://codereview.chromium.org/2154873002/diff/1/blimp/engine/testing/Docker... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2154873002/diff/1/blimp/engine/testing/Docker... blimp/engine/testing/Dockerfile:13: # Docker copies directory CONTENTS rather than the directories themselves. On 2016/07/15 20:26:59, Jess wrote: > Note for reviewers. This is the point of confusion that existed previously for > the Dockerfiles. > > Documentation https://docs.docker.com/engine/reference/builder/#/add > > Each <src> may contain wildcards and matching will be done using Go’s > filepath.Match rules. > ... > If <src> is a directory, the entire contents of the directory are copied, > including filesystem metadata. > Note: The directory itself is not copied, just its contents. I want to understand what's the desired directory structure inside the docker image. Do you really want sub-directories for net, base, etc.? Is seems like the simplest thing would be for everything in the tarball to end up in /test, right? I think you can achieve that with "ADD . /test/". For example: $ cat Dockerfile FROM ubuntu:trusty RUN mkdir /foo ADD . /foo/ ENTRYPOINT [ "/bin/bash" ] $ tar czf ../files.tgz ./ && cd .. && docker build - < files.tgz Sending build context to Docker daemon 3.072 kB Step 1 : FROM ubuntu:trusty ---> f429597fd334 Step 2 : RUN mkdir /foo ---> Using cache ---> 552fa8b209da Step 3 : ADD . /foo/ ---> Using cache ---> 830b6f8538a2 Step 4 : ENTRYPOINT /bin/bash ---> Using cache ---> fed1e3d7fc92 Successfully built fed1e3d7fc92 $ docker run -ti fed1e3d7fc92 root@6132083cf5d7:/# ls -laFR /foo /foo: total 16 drwxr-xr-x 3 root root 4096 Jul 15 20:48 ./ drwxr-xr-x 35 root root 4096 Jul 15 20:50 ../ -rw-r----- 1 root root 73 Jul 15 20:47 Dockerfile drwxr-x--- 2 root root 4096 Jul 15 20:34 one/ /foo/one: total 8 drwxr-x--- 2 root root 4096 Jul 15 20:34 ./ drwxr-xr-x 3 root root 4096 Jul 15 20:48 ../ -rw-r----- 1 root root 0 Jul 15 20:34 file root@6132083cf5d7:/#
https://codereview.chromium.org/2154873002/diff/1/blimp/engine/testing/Docker... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2154873002/diff/1/blimp/engine/testing/Docker... blimp/engine/testing/Dockerfile:13: # Docker copies directory CONTENTS rather than the directories themselves. On 2016/07/15 20:54:01, maniscalco wrote: > On 2016/07/15 20:26:59, Jess wrote: > > Note for reviewers. This is the point of confusion that existed previously for > > the Dockerfiles. > > > > Documentation https://docs.docker.com/engine/reference/builder/#/add > > > > Each <src> may contain wildcards and matching will be done using Go’s > > filepath.Match rules. > > ... > > If <src> is a directory, the entire contents of the directory are copied, > > including filesystem metadata. > > Note: The directory itself is not copied, just its contents. > > I want to understand what's the desired directory structure inside the docker > image. > > Do you really want sub-directories for net, base, etc.? > > Is seems like the simplest thing would be for everything in the tarball to end > up in /test, right? I think you can achieve that with "ADD . /test/". For > example: > > $ cat Dockerfile > FROM ubuntu:trusty > RUN mkdir /foo > ADD . /foo/ > ENTRYPOINT [ "/bin/bash" ] > > > $ tar czf ../files.tgz ./ && cd .. && docker build - < files.tgz > Sending build context to Docker daemon 3.072 kB > Step 1 : FROM ubuntu:trusty > ---> f429597fd334 > Step 2 : RUN mkdir /foo > ---> Using cache > ---> 552fa8b209da > Step 3 : ADD . /foo/ > ---> Using cache > ---> 830b6f8538a2 > Step 4 : ENTRYPOINT /bin/bash > ---> Using cache > ---> fed1e3d7fc92 > Successfully built fed1e3d7fc92 > > $ docker run -ti fed1e3d7fc92 > root@6132083cf5d7:/# ls -laFR /foo > /foo: > total 16 > drwxr-xr-x 3 root root 4096 Jul 15 20:48 ./ > drwxr-xr-x 35 root root 4096 Jul 15 20:50 ../ > -rw-r----- 1 root root 73 Jul 15 20:47 Dockerfile > drwxr-x--- 2 root root 4096 Jul 15 20:34 one/ > > /foo/one: > total 8 > drwxr-x--- 2 root root 4096 Jul 15 20:34 ./ > drwxr-xr-x 3 root root 4096 Jul 15 20:48 ../ > -rw-r----- 1 root root 0 Jul 15 20:34 file > root@6132083cf5d7:/# > +1 asked the same question offline to Jess. I don't think we want a subdirectory for each of the test type but just one out/test.
https://codereview.chromium.org/2154873002/diff/1/blimp/engine/testing/Docker... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2154873002/diff/1/blimp/engine/testing/Docker... blimp/engine/testing/Dockerfile:13: # Docker copies directory CONTENTS rather than the directories themselves. On 2016/07/15 22:23:09, Sriram wrote: > On 2016/07/15 20:54:01, maniscalco wrote: > > On 2016/07/15 20:26:59, Jess wrote: > > > Note for reviewers. This is the point of confusion that existed previously > for > > > the Dockerfiles. > > > > > > Documentation https://docs.docker.com/engine/reference/builder/#/add > > > > > > Each <src> may contain wildcards and matching will be done using Go’s > > > filepath.Match rules. > > > ... > > > If <src> is a directory, the entire contents of the directory are copied, > > > including filesystem metadata. > > > Note: The directory itself is not copied, just its contents. > > > > I want to understand what's the desired directory structure inside the docker > > image. > > > > Do you really want sub-directories for net, base, etc.? > > > > Is seems like the simplest thing would be for everything in the tarball to end > > up in /test, right? I think you can achieve that with "ADD . /test/". For > > example: > > > > $ cat Dockerfile > > FROM ubuntu:trusty > > RUN mkdir /foo > > ADD . /foo/ > > ENTRYPOINT [ "/bin/bash" ] > > > > > > $ tar czf ../files.tgz ./ && cd .. && docker build - < files.tgz > > Sending build context to Docker daemon 3.072 kB > > Step 1 : FROM ubuntu:trusty > > ---> f429597fd334 > > Step 2 : RUN mkdir /foo > > ---> Using cache > > ---> 552fa8b209da > > Step 3 : ADD . /foo/ > > ---> Using cache > > ---> 830b6f8538a2 > > Step 4 : ENTRYPOINT /bin/bash > > ---> Using cache > > ---> fed1e3d7fc92 > > Successfully built fed1e3d7fc92 > > > > $ docker run -ti fed1e3d7fc92 > > root@6132083cf5d7:/# ls -laFR /foo > > /foo: > > total 16 > > drwxr-xr-x 3 root root 4096 Jul 15 20:48 ./ > > drwxr-xr-x 35 root root 4096 Jul 15 20:50 ../ > > -rw-r----- 1 root root 73 Jul 15 20:47 Dockerfile > > drwxr-x--- 2 root root 4096 Jul 15 20:34 one/ > > > > /foo/one: > > total 8 > > drwxr-x--- 2 root root 4096 Jul 15 20:34 ./ > > drwxr-xr-x 3 root root 4096 Jul 15 20:48 ../ > > -rw-r----- 1 root root 0 Jul 15 20:34 file > > root@6132083cf5d7:/# > > > > +1 asked the same question offline to Jess. I don't think we want a subdirectory > for each of the test type but just one out/test. Resolved issues from here: Use ADD . /blimp for src directory deps. Resolved issues discussed offline: Update base directory name from "test" to "blimp" Issues from offline still to be addressed: representing the build directory structure in the tarball itself so we don't have to fix it using this Dockerfile. I'll send a mail when that last one is in place, but wanted to capture the offline and Dockerfile changes immediately
Move filesystem manipulations for tests into the bundle creation script.
The CQ bit was checked by jessicag@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...
Description was changed from ========== Fix ADD handling in Dockerfiles and associated comments. Current Dockerfiles were written with a misunderstanding of how wildcards and directory sources interact with the ADD command. This was discovered during testing of the testing export. The change fixes an incorrect comment in the engine Dockerfile and updates the testing Dockerfile to construct the filesystem as required for testing. This change does not remove the duplicate ./third_party/blimp_fonts (./fonts is used instead) for the engine environment. BUG=616945, 608487 ========== to ========== Fix ADD handling in Dockerfiles and test bundle creation. Current Dockerfiles were written with a misunderstanding of how wildcards and directory sources interact with the ADD command. This was discovered during testing of the testing export. The change fixes an incorrect comment in the engine Dockerfile. The testing Dockerfile is updated to export the filesystem as it exists in the uploaded tarball. This change also updates the create_bundle script to support the creation of a tarball with the desired filesystem structure for testing. This change does not remove the duplicate ./third_party/blimp_fonts (./fonts is used instead) for the engine environment. BUG=616945, 608487 ==========
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jessicag@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...
https://codereview.chromium.org/2154873002/diff/60001/blimp/engine/Dockerfile File blimp/engine/Dockerfile (right): https://codereview.chromium.org/2154873002/diff/60001/blimp/engine/Dockerfile... blimp/engine/Dockerfile:8: # contents of any subdirectory there. *scratches head* How has the behaviour changed if the script commands haven't? https://codereview.chromium.org/2154873002/diff/60001/blimp/tools/create-bund... File blimp/tools/create-bundle.py (right): https://codereview.chromium.org/2154873002/diff/60001/blimp/tools/create-bund... blimp/tools/create-bundle.py:18: def ReadDependencies(manifest, relpath): Here & below, Python Style Guide is to use unix_hacker_style, so this should be something like relative_path or dependency_path or possibly deps_path (though dep[s] should really be renamed as well).
The CQ bit was checked by jessicag@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...
https://codereview.chromium.org/2154873002/diff/60001/blimp/engine/Dockerfile File blimp/engine/Dockerfile (right): https://codereview.chromium.org/2154873002/diff/60001/blimp/engine/Dockerfile... blimp/engine/Dockerfile:8: # contents of any subdirectory there. On 2016/07/16 00:26:36, Wez wrote: > *scratches head* > > How has the behaviour changed if the script commands haven't? The behavior didn't change. The comment does not correctly describe the behavior of docker. At least not in the current version. https://codereview.chromium.org/2154873002/diff/60001/blimp/tools/create-bund... File blimp/tools/create-bundle.py (right): https://codereview.chromium.org/2154873002/diff/60001/blimp/tools/create-bund... blimp/tools/create-bundle.py:18: def ReadDependencies(manifest, relpath): On 2016/07/16 00:26:36, Wez wrote: > Here & below, Python Style Guide is to use unix_hacker_style, so this should be > something like relative_path or dependency_path or possibly deps_path (though > dep[s] should really be renamed as well). Updated the variable names you mentioned.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2154873002/diff/80001/blimp/engine/Dockerfile File blimp/engine/Dockerfile (right): https://codereview.chromium.org/2154873002/diff/80001/blimp/engine/Dockerfile... blimp/engine/Dockerfile:8: # contents of any subdirectory there. nit: I'd probably just skip this kind of comment altogether because it's a "what" comment and not a "why" comment. If someone wants to know what this does, they can (and probably should) just go read the docker doc. https://codereview.chromium.org/2154873002/diff/80001/blimp/engine/testing/Do... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2154873002/diff/80001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:5: FROM base:latest Can you please attached an example of the tar file contents (i.e. tar tzf output)? Doesn't have to be the full listing, just enough for someone to review the change. Can you do this for both the engine and the test bundles? https://codereview.chromium.org/2154873002/diff/80001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:7: RUN mkdir -p /blimp/out/test I realize we need to mkdir /blimp, but do we need to make out/test as well? Won't those be created automatically when we ADD further down? https://codereview.chromium.org/2154873002/diff/80001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:11: # Copy complete docker root dir into /blimp directory. Can you change this to explain why we're copying things they way we are? For example: # Put all the blimp related files in /blimp so they are kept separate from the OS files. Some of these executable make use of data using paths relative to the executable's path so add "." to preserve input directory structure. https://codereview.chromium.org/2154873002/diff/80001/blimp/tools/create-bund... File blimp/tools/create-bundle.py (right): https://codereview.chromium.org/2154873002/diff/80001/blimp/tools/create-bund... blimp/tools/create-bundle.py:19: """Read the manifest and return the list of dependencies. Docs of this function need to be updated now that it does more than just read and return the list. The doc string should explain that it prefixes each returned item with relative_path (and normalizes the prefixed path?). https://codereview.chromium.org/2154873002/diff/80001/blimp/tools/create-bund... blimp/tools/create-bundle.py:55: parser.add_argument('--root-dir', Let's say the manifest (really the runtime deps file) contains: foo/bar/baz ./buzz ../../datafile If I call this program with build-dir=/build and don't specify --root-dir, the tar will contain: foo/bar/baz ./buzz ../../datafile right? Now if I were to pass apple/banana as --root-dir, I'd get: apple/banana/foo/bar/baz apple/banana/buzz datafile right? I'm not sure the help text for this arg fully conveys its purpose. It might be more obvious if you gave it a more descriptive name. For example: --with-tar-contents-relative-to --with-tar-contents-rooted-in As for the help text, how about something like: 'optional path prefix to use inside the resulting tar file.' ?
The CQ bit was checked by jessicag@chromium.org to run a CQ dry run
https://codereview.chromium.org/2154873002/diff/80001/blimp/engine/Dockerfile File blimp/engine/Dockerfile (right): https://codereview.chromium.org/2154873002/diff/80001/blimp/engine/Dockerfile... blimp/engine/Dockerfile:8: # contents of any subdirectory there. On 2016/07/18 16:01:04, maniscalco wrote: > nit: I'd probably just skip this kind of comment altogether because it's a > "what" comment and not a "why" comment. If someone wants to know what this > does, they can (and probably should) just go read the docker doc. Done. https://codereview.chromium.org/2154873002/diff/80001/blimp/engine/testing/Do... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2154873002/diff/80001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:5: FROM base:latest On 2016/07/18 16:01:04, maniscalco wrote: > Can you please attached an example of the tar file contents (i.e. tar tzf > output)? Doesn't have to be the full listing, just enough for someone to review > the change. Can you do this for both the engine and the test bundles? Sure. Directories with large expansions are summaryized with ... at the base of the expansion. Note that the build directory prefix for the testing bundle ("out-linux/Debug" here) is specific to the build. $ tar -tzf out-linux/Debug/blimp_engine_bundle.tar.gz blimp_engine_app blimp_engine.pak chrome_sandbox gen/third_party/blimp_fonts/... icudtl.dat natives_blob.bin libui_library.so Dockerfile Dockerfile.base start_engine.sh $tar -tzf out-linux/Debug/blimp_engine_env_tests_bundle.tar.gz ut-linux/Debug/blimp_unittests out-linux/Debug/icudtl.dat out-linux/Debug/natives_blob.bin out-linux/Debug/libui_library.so blimp/client/session/test_selfsigned_cert.pem blimp/test/data/test_client_token net/data/... out-linux/Debug/base_unittests base/test/data/... out-linux/Debug/libmalloc_wrapper.so Dockerfile Dockerfile.base https://codereview.chromium.org/2154873002/diff/80001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:7: RUN mkdir -p /blimp/out/test On 2016/07/18 16:01:04, maniscalco wrote: > I realize we need to mkdir /blimp, but do we need to make out/test as well? > Won't those be created automatically when we ADD further down? Done. https://codereview.chromium.org/2154873002/diff/80001/blimp/engine/testing/Do... blimp/engine/testing/Dockerfile:11: # Copy complete docker root dir into /blimp directory. On 2016/07/18 16:01:04, maniscalco wrote: > Can you change this to explain why we're copying things they way we are? For > example: > > # Put all the blimp related files in /blimp so they are kept separate from the > OS files. Some of these executable make use of data using paths relative to the > executable's path so add "." to preserve input directory structure. Done.
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jessicag@chromium.org to run a CQ dry run
https://codereview.chromium.org/2154873002/diff/80001/blimp/tools/create-bund... File blimp/tools/create-bundle.py (right): https://codereview.chromium.org/2154873002/diff/80001/blimp/tools/create-bund... blimp/tools/create-bundle.py:19: """Read the manifest and return the list of dependencies. On 2016/07/18 16:01:05, maniscalco wrote: > Docs of this function need to be updated now that it does more than just read > and return the list. The doc string should explain that it prefixes each > returned item with relative_path (and normalizes the prefixed path?). Done. https://codereview.chromium.org/2154873002/diff/80001/blimp/tools/create-bund... blimp/tools/create-bundle.py:55: parser.add_argument('--root-dir', On 2016/07/18 16:01:05, maniscalco wrote: > Let's say the manifest (really the runtime deps file) contains: > > foo/bar/baz > ./buzz > ../../datafile > > If I call this program with build-dir=/build and don't specify --root-dir, the > tar will contain: > > foo/bar/baz > ./buzz > ../../datafile > > right? > > Now if I were to pass apple/banana as --root-dir, I'd get: > > apple/banana/foo/bar/baz > apple/banana/buzz > datafile > > right? > > > > I'm not sure the help text for this arg fully conveys its purpose. It might be > more obvious if you gave it a more descriptive name. For example: > > --with-tar-contents-relative-to > --with-tar-contents-rooted-in > > > As for the help text, how about something like: > > 'optional path prefix to use inside the resulting tar file.' ? Updated. PTAL. Agree this value's meaning is not the simplest concept to convey.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM % a few more comments. https://codereview.chromium.org/2154873002/diff/120001/blimp/engine/testing/D... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2154873002/diff/120001/blimp/engine/testing/D... blimp/engine/testing/Dockerfile:11: # Put all the blimp related files in /blimp so they are kept separate from nit: blimp -> Blimp for consistency with other comments in this file. https://codereview.chromium.org/2154873002/diff/120001/blimp/engine/testing/D... blimp/engine/testing/Dockerfile:13: # maintained since ADD only copies the contents of directories nit: Missing period at end of sentence. https://codereview.chromium.org/2154873002/diff/120001/blimp/tools/create-bun... File blimp/tools/create-bundle.py (right): https://codereview.chromium.org/2154873002/diff/120001/blimp/tools/create-bun... blimp/tools/create-bundle.py:19: """Read the manifest. Takes each dependency found within prepends it with nit: Need a comma between "within" and "prepends". https://codereview.chromium.org/2154873002/diff/120001/blimp/tools/create-bun... blimp/tools/create-bundle.py:31: if relative_path != '': I'm no python expert, but I believe the idiomatic way to check for empty string is: if not relative_path: (similar to how we do "if dependency" above to check for non-empty string) https://codereview.chromium.org/2154873002/diff/120001/blimp/tools/create-bun... blimp/tools/create-bundle.py:59: 'tar file.'), nit: Remove the period at the end of the help text to be consistent with the other help text above. https://codereview.chromium.org/2154873002/diff/120001/blimp/tools/create-bun... blimp/tools/create-bundle.py:67: relative_path = os.path.relpath(args.build_dir, dependencies_path) So relative_path is always non-empty, right? If that's the case, can you remove the check from ReadDependencies above? In other words, the second argument to ReadDependencies is no longer optional, right?
The CQ bit was checked by jessicag@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 checked by jessicag@chromium.org to run a CQ dry run
Thanks for hanging on Wez. Please review for OWNERs approval. https://codereview.chromium.org/2154873002/diff/120001/blimp/engine/testing/D... File blimp/engine/testing/Dockerfile (right): https://codereview.chromium.org/2154873002/diff/120001/blimp/engine/testing/D... blimp/engine/testing/Dockerfile:11: # Put all the blimp related files in /blimp so they are kept separate from On 2016/07/19 21:52:36, maniscalco wrote: > nit: blimp -> Blimp for consistency with other comments in this file. Done. https://codereview.chromium.org/2154873002/diff/120001/blimp/engine/testing/D... blimp/engine/testing/Dockerfile:13: # maintained since ADD only copies the contents of directories On 2016/07/19 21:52:36, maniscalco wrote: > nit: Missing period at end of sentence. Done. https://codereview.chromium.org/2154873002/diff/120001/blimp/tools/create-bun... File blimp/tools/create-bundle.py (right): https://codereview.chromium.org/2154873002/diff/120001/blimp/tools/create-bun... blimp/tools/create-bundle.py:19: """Read the manifest. Takes each dependency found within prepends it with On 2016/07/19 21:52:36, maniscalco wrote: > nit: Need a comma between "within" and "prepends". Done. https://codereview.chromium.org/2154873002/diff/120001/blimp/tools/create-bun... blimp/tools/create-bundle.py:31: if relative_path != '': On 2016/07/19 21:52:36, maniscalco wrote: > I'm no python expert, but I believe the idiomatic way to check for empty string > is: > > if not relative_path: > > (similar to how we do "if dependency" above to check for non-empty string) Acknowledged. Comment below overrides. https://codereview.chromium.org/2154873002/diff/120001/blimp/tools/create-bun... blimp/tools/create-bundle.py:59: 'tar file.'), On 2016/07/19 21:52:36, maniscalco wrote: > nit: Remove the period at the end of the help text to be consistent with the > other help text above. Done. https://codereview.chromium.org/2154873002/diff/120001/blimp/tools/create-bun... blimp/tools/create-bundle.py:67: relative_path = os.path.relpath(args.build_dir, dependencies_path) On 2016/07/19 21:52:36, maniscalco wrote: > So relative_path is always non-empty, right? If that's the case, can you remove > the check from ReadDependencies above? In other words, the second argument to > ReadDependencies is no longer optional, right? Ah, even cleaner.
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Reminder ping.
https://codereview.chromium.org/2154873002/diff/160001/blimp/tools/create-bun... File blimp/tools/create-bundle.py (left): https://codereview.chromium.org/2154873002/diff/160001/blimp/tools/create-bun... blimp/tools/create-bundle.py:68: "-C", args.build_dir] + deps I am a little confused... Is the final structure of the tarball be: Source -> Target out/Debug/foo -> out/test/food out/Debug/icudtl.dat -> out/test/icudtl.dat out/Debug/base -> base and so on?
nit: In the description "testing of the testing export" reads oddly and it wasn't clear to me what "testing export" refers to - just the package produced by the testing Dockerfile?
On 2016/07/22 00:38:52, Sriram wrote: > https://codereview.chromium.org/2154873002/diff/160001/blimp/tools/create-bun... > File blimp/tools/create-bundle.py (left): > > https://codereview.chromium.org/2154873002/diff/160001/blimp/tools/create-bun... > blimp/tools/create-bundle.py:68: "-C", args.build_dir] + deps > I am a little confused... Is the final structure of the tarball be: > > Source -> Target > out/Debug/foo -> out/test/food > out/Debug/icudtl.dat -> out/test/icudtl.dat > out/Debug/base -> base > > and so on? Here is what is currently generated with a build directory of out-linux/Debug. $ tar -tzf out-linux/Debug/blimp_engine_bundle.tar.gz blimp_engine_app blimp_engine.pak chrome_sandbox gen/third_party/blimp_fonts/... icudtl.dat natives_blob.bin libui_library.so Dockerfile Dockerfile.base start_engine.sh $tar -tzf out-linux/Debug/blimp_engine_env_tests_bundle.tar.gz ut-linux/Debug/blimp_unittests out-linux/Debug/icudtl.dat out-linux/Debug/natives_blob.bin out-linux/Debug/libui_library.so blimp/client/session/test_selfsigned_cert.pem blimp/test/data/test_client_token net/data/... out-linux/Debug/base_unittests base/test/data/... out-linux/Debug/libmalloc_wrapper.so Dockerfile Dockerfile.base Before everything under out-linux/Debug/ was in the tarball root.
lgtm https://codereview.chromium.org/2154873002/diff/160001/blimp/tools/create-bun... File blimp/tools/create-bundle.py (right): https://codereview.chromium.org/2154873002/diff/160001/blimp/tools/create-bun... blimp/tools/create-bundle.py:19: """Read the manifest. Takes each dependency found within, prepends it with nit: Shouldn't the description start with "Returns a list of dependencies.."? e.g: "Returns a list of dependencies based on the specified manifest file. The returned dependencies will be made relative to |relative_path| if specified, and normalized." Also, we call os.path.join(relative_path,...) unconditionally, so is it correct to say that the relative_path is optional?
Description was changed from ========== Fix ADD handling in Dockerfiles and test bundle creation. Current Dockerfiles were written with a misunderstanding of how wildcards and directory sources interact with the ADD command. This was discovered during testing of the testing export. The change fixes an incorrect comment in the engine Dockerfile. The testing Dockerfile is updated to export the filesystem as it exists in the uploaded tarball. This change also updates the create_bundle script to support the creation of a tarball with the desired filesystem structure for testing. This change does not remove the duplicate ./third_party/blimp_fonts (./fonts is used instead) for the engine environment. BUG=616945, 608487 ========== to ========== Update ADD handling in Dockerfiles and test bundle creation. Current Dockerfiles were written with a misunderstanding of how wildcards and directory sources interact with the ADD command. This was discovered in testing using the generated tarballs. The change fixes an incorrect comment in the engine Dockerfile. Additionally, create_bundle.py was updated to tar up the filesystem state from a context other than the build directory. These changes mean that for the env_tests bundle, running the Dockerfile in the context of the tarball results in a system file structure such that /blimp/ contains the set of dpendencies for the testing binaries in the same directory structure as // (the chromium src directory). This change does not remove the duplicate ./third_party/blimp_fonts (./fonts is used instead) for the engine environment. BUG=616945, 608487 ==========
The CQ bit was checked by jessicag@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...
https://codereview.chromium.org/2154873002/diff/160001/blimp/tools/create-bun... File blimp/tools/create-bundle.py (left): https://codereview.chromium.org/2154873002/diff/160001/blimp/tools/create-bun... blimp/tools/create-bundle.py:68: "-C", args.build_dir] + deps On 2016/07/22 00:38:51, Sriram wrote: > I am a little confused... Is the final structure of the tarball be: > > Source -> Target > out/Debug/foo -> out/test/food > out/Debug/icudtl.dat -> out/test/icudtl.dat > out/Debug/base -> base > > and so on? Here is what is currently generated with a build directory of out-linux/Debug. $ tar -tzf out-linux/Debug/blimp_engine_bundle.tar.gz blimp_engine_app blimp_engine.pak chrome_sandbox gen/third_party/blimp_fonts/... icudtl.dat natives_blob.bin libui_library.so Dockerfile Dockerfile.base start_engine.sh $tar -tzf out-linux/Debug/blimp_engine_env_tests_bundle.tar.gz ut-linux/Debug/blimp_unittests out-linux/Debug/icudtl.dat out-linux/Debug/natives_blob.bin out-linux/Debug/libui_library.so blimp/client/session/test_selfsigned_cert.pem blimp/test/data/test_client_token net/data/... out-linux/Debug/base_unittests base/test/data/... out-linux/Debug/libmalloc_wrapper.so Dockerfile Dockerfile.base Before everything under out-linux/Debug/ was in the tarball root. https://codereview.chromium.org/2154873002/diff/160001/blimp/tools/create-bun... File blimp/tools/create-bundle.py (right): https://codereview.chromium.org/2154873002/diff/160001/blimp/tools/create-bun... blimp/tools/create-bundle.py:19: """Read the manifest. Takes each dependency found within, prepends it with On 2016/07/22 00:50:28, Wez wrote: > nit: Shouldn't the description start with "Returns a list of dependencies.."? > e.g: > > "Returns a list of dependencies based on the specified manifest file. The > returned dependencies will be made relative to |relative_path| if specified, and > normalized." > > Also, we call os.path.join(relative_path,...) unconditionally, so is it correct > to say that the relative_path is optional? Good points. I didn't update the comments with the most recent set of edits to the file in the cl. Fixed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Update ADD handling in Dockerfiles and test bundle creation. Current Dockerfiles were written with a misunderstanding of how wildcards and directory sources interact with the ADD command. This was discovered in testing using the generated tarballs. The change fixes an incorrect comment in the engine Dockerfile. Additionally, create_bundle.py was updated to tar up the filesystem state from a context other than the build directory. These changes mean that for the env_tests bundle, running the Dockerfile in the context of the tarball results in a system file structure such that /blimp/ contains the set of dpendencies for the testing binaries in the same directory structure as // (the chromium src directory). This change does not remove the duplicate ./third_party/blimp_fonts (./fonts is used instead) for the engine environment. BUG=616945, 608487 ========== to ========== Update ADD handling in Dockerfiles and test bundle creation. Current Dockerfiles were written with a misunderstanding of how wildcards and directory sources interact with the ADD command. This was discovered in testing using the generated tarballs. The change fixes an incorrect comment in the engine Dockerfile. Additionally, create_bundle.py was updated to tar up the filesystem state from a context other than the build directory. These changes mean that for the env_tests bundle, running the Dockerfile in the context of the tarball results in a system file structure such that /blimp/ contains the set of dpendencies for the testing binaries in the same directory structure as // (the chromium src directory). This change does not remove the duplicate ./third_party/blimp_fonts (./fonts is used instead) for the engine environment. BUG=616945, 608487, 630438 ==========
lgtm
The CQ bit was checked by jessicag@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maniscalco@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2154873002/#ps180001 (title: "Update create-bundle.py comments to reflect most recent behavior changes.")
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 ========== Update ADD handling in Dockerfiles and test bundle creation. Current Dockerfiles were written with a misunderstanding of how wildcards and directory sources interact with the ADD command. This was discovered in testing using the generated tarballs. The change fixes an incorrect comment in the engine Dockerfile. Additionally, create_bundle.py was updated to tar up the filesystem state from a context other than the build directory. These changes mean that for the env_tests bundle, running the Dockerfile in the context of the tarball results in a system file structure such that /blimp/ contains the set of dpendencies for the testing binaries in the same directory structure as // (the chromium src directory). This change does not remove the duplicate ./third_party/blimp_fonts (./fonts is used instead) for the engine environment. BUG=616945, 608487, 630438 ========== to ========== Update ADD handling in Dockerfiles and test bundle creation. Current Dockerfiles were written with a misunderstanding of how wildcards and directory sources interact with the ADD command. This was discovered in testing using the generated tarballs. The change fixes an incorrect comment in the engine Dockerfile. Additionally, create_bundle.py was updated to tar up the filesystem state from a context other than the build directory. These changes mean that for the env_tests bundle, running the Dockerfile in the context of the tarball results in a system file structure such that /blimp/ contains the set of dpendencies for the testing binaries in the same directory structure as // (the chromium src directory). This change does not remove the duplicate ./third_party/blimp_fonts (./fonts is used instead) for the engine environment. BUG=616945, 608487, 630438 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Update ADD handling in Dockerfiles and test bundle creation. Current Dockerfiles were written with a misunderstanding of how wildcards and directory sources interact with the ADD command. This was discovered in testing using the generated tarballs. The change fixes an incorrect comment in the engine Dockerfile. Additionally, create_bundle.py was updated to tar up the filesystem state from a context other than the build directory. These changes mean that for the env_tests bundle, running the Dockerfile in the context of the tarball results in a system file structure such that /blimp/ contains the set of dpendencies for the testing binaries in the same directory structure as // (the chromium src directory). This change does not remove the duplicate ./third_party/blimp_fonts (./fonts is used instead) for the engine environment. BUG=616945, 608487, 630438 ========== to ========== Update ADD handling in Dockerfiles and test bundle creation. Current Dockerfiles were written with a misunderstanding of how wildcards and directory sources interact with the ADD command. This was discovered in testing using the generated tarballs. The change fixes an incorrect comment in the engine Dockerfile. Additionally, create_bundle.py was updated to tar up the filesystem state from a context other than the build directory. These changes mean that for the env_tests bundle, running the Dockerfile in the context of the tarball results in a system file structure such that /blimp/ contains the set of dpendencies for the testing binaries in the same directory structure as // (the chromium src directory). This change does not remove the duplicate ./third_party/blimp_fonts (./fonts is used instead) for the engine environment. BUG=616945, 608487, 630438 Committed: https://crrev.com/6df4245122709b6a19e5f898d49a777b153c9bd7 Cr-Commit-Position: refs/heads/master@{#407237} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/6df4245122709b6a19e5f898d49a777b153c9bd7 Cr-Commit-Position: refs/heads/master@{#407237} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
