Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2574033002:
Downloading Node and NPM deps via gclient sync. (Closed)
DescriptionDownloading Node and NPM deps via gclient sync.
This is necessary for WebUI to harness several Node based tools
to speed up performance, as well as remove generated code that is
currently checked in as source code.
Discussion occurred at:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/H2IqgqwdUqs/yn_6_z0cDwAJ
BUG=673825
Review-Url: https://codereview.chromium.org/2574033002
Cr-Commit-Position: refs/heads/master@{#443777}
Committed: https://chromium.googlesource.com/chromium/src/+/cbee6ab37edb7df0d8d5354fd2af29e968f33e94
Patch Set 1 : update gitignore #Patch Set 2 : Add windows. #Patch Set 3 : Merge conflicts. #Patch Set 4 : Add README.chromium #Patch Set 5 : fix win #Patch Set 6 : Add LICENSE file #Patch Set 7 : expect checklicenses to blow up. #Patch Set 8 : Pull down NPM dependencies too. #Patch Set 9 : Avoid unnecessary re-downloading . #Patch Set 10 : Reduce NPM deps size #
Total comments: 4
Patch Set 11 : Add update scripts #
Total comments: 7
Patch Set 12 : Address comments + some fixes. #Patch Set 13 : Nit #
Total comments: 10
Patch Set 14 : Nits. #Patch Set 15 : Add quotes around bash variables. #Patch Set 16 : Update node_modules sha1 #
Total comments: 7
Patch Set 17 : Address comments. #
Total comments: 2
Patch Set 18 : Nits. #
Total comments: 6
Patch Set 19 : Nits. #Patch Set 20 : Add missing sha1 file. #
Messages
Total messages: 81 (50 generated)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by dpapad@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 dpapad@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 ========== Add setup for downloading Node via gclient sync, WIP. BUG=673825 ========== to ========== Downloading Node via gclient sync. This is necessary for WebUI. BUG=673825 ==========
Description was changed from ========== Downloading Node via gclient sync. This is necessary for WebUI. BUG=673825 ========== to ========== Downloading Node via gclient sync. This is necessary for WebUI to harness several Node based tools to speed up performance (as well as remove generated code from the repository). BUG=673825 ==========
Description was changed from ========== Downloading Node via gclient sync. This is necessary for WebUI to harness several Node based tools to speed up performance (as well as remove generated code from the repository). BUG=673825 ========== to ========== Downloading Node via gclient sync. This is necessary for WebUI to harness several Node based tools to speed up performance, as well as remove generated code that is currently checked in as source code. BUG=673825 ==========
The CQ bit was checked by dpapad@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.
Description was changed from ========== Downloading Node via gclient sync. This is necessary for WebUI to harness several Node based tools to speed up performance, as well as remove generated code that is currently checked in as source code. BUG=673825 ========== to ========== Downloading Node and NPM deps via gclient sync. This is necessary for WebUI to harness several Node based tools to speed up performance, as well as remove generated code that is currently checked in as source code. BUG=673825 ==========
The CQ bit was checked by dpapad@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 dpapad@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 dpapad@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 ========== Downloading Node and NPM deps via gclient sync. This is necessary for WebUI to harness several Node based tools to speed up performance, as well as remove generated code that is currently checked in as source code. BUG=673825 ========== to ========== Downloading Node and NPM deps via gclient sync. This is necessary for WebUI to harness several Node based tools to speed up performance, as well as remove generated code that is currently checked in as source code. Discussion occurred at: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/H2IqgqwdUqs/yn_6_... BUG=673825 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
This basically looks okay, except we need to know how to be able to recreate or update these things. https://codereview.chromium.org/2574033002/diff/240001/third_party/node/READM... File third_party/node/README.chromium (right): https://codereview.chromium.org/2574033002/diff/240001/third_party/node/READM... third_party/node/README.chromium:3: URL: https://github.com/joyent/node Not https://github.com/nodejs/node ? https://codereview.chromium.org/2574033002/diff/240001/third_party/node/READM... third_party/node/README.chromium:12: Node binary and NPM modules necessary for buliding Chrome's WebUI. You need to document how to recreate or re-retrieve the binaries and modules.
Patchset #11 (id:260001) has been deleted
dpapad@chromium.org changed reviewers: + dbeam@chromium.org, security@chromium.org
dbeam: Feel free to suggest bash scripts improvements. https://codereview.chromium.org/2574033002/diff/240001/third_party/node/READM... File third_party/node/README.chromium (right): https://codereview.chromium.org/2574033002/diff/240001/third_party/node/READM... third_party/node/README.chromium:3: URL: https://github.com/joyent/node On 2017/01/09 at 02:09:48, Dirk Pranke wrote: > Not https://github.com/nodejs/node ? Done. https://codereview.chromium.org/2574033002/diff/240001/third_party/node/READM... third_party/node/README.chromium:12: Node binary and NPM modules necessary for buliding Chrome's WebUI. On 2017/01/09 at 02:09:48, Dirk Pranke wrote: > You need to document how to recreate or re-retrieve the binaries and modules. Added two scripts in this CL, update_node_binaries.sh and update_npm_deps.sh. I was initially planning to land those separately, to make this review easier. https://codereview.chromium.org/2574033002/diff/280001/third_party/node/updat... File third_party/node/update_node_binaries.sh (right): https://codereview.chromium.org/2574033002/diff/280001/third_party/node/updat... third_party/node/update_node_binaries.sh:24: tar cfz ${FOLDER}/node-darwin-x64.tar.gz ${FOLDER}/node-${NODE_VERSION}-darwin-x64/ FWIW, this command is not 100% deterministic (the output file has a different sha1 hash every time). Not necessarily a problem, but if there is an easy way to fix this it would be nice. https://codereview.chromium.org/2574033002/diff/280001/third_party/node/updat... third_party/node/update_node_binaries.sh:62: sha1="$(sha1sum ${FOLDER}/node.exe | cut -d ' ' -f1)" Perhaps we should also compress node.exe? I did not initially do it because I did not know if the download_from_google_storage.py --extract option would work on windows (it does).
scottchen@chromium.org changed reviewers: + scottchen@chromium.org
https://codereview.chromium.org/2574033002/diff/280001/third_party/node/npm_e... File third_party/node/npm_exclude.txt (right): https://codereview.chromium.org/2574033002/diff/280001/third_party/node/npm_e... third_party/node/npm_exclude.txt:8: *.png would we run into issues excluding .png, .svg, and fonts if a front-end library's trying to use them in its css? I'm thinking bootstrap icons as an example.
https://codereview.chromium.org/2574033002/diff/280001/third_party/node/npm_e... File third_party/node/npm_exclude.txt (right): https://codereview.chromium.org/2574033002/diff/280001/third_party/node/npm_e... third_party/node/npm_exclude.txt:8: *.png On 2017/01/10 21:56:05, scottchen wrote: > would we run into issues excluding .png, .svg, and fonts if a front-end > library's trying to use them in its css? I'm thinking bootstrap icons as an > example. but we're not doing that yet and this list can change to accommodate that if we ever did (for some reason) https://codereview.chromium.org/2574033002/diff/280001/third_party/node/updat... File third_party/node/update_node_binaries.sh (right): https://codereview.chromium.org/2574033002/diff/280001/third_party/node/updat... third_party/node/update_node_binaries.sh:62: sha1="$(sha1sum ${FOLDER}/node.exe | cut -d ' ' -f1)" On 2017/01/10 19:59:49, dpapad wrote: > Perhaps we should also compress node.exe? I did not initially do it because I > did not know if the download_from_google_storage.py --extract option would work > on windows (it does). i suspect that would not be useful
you might want to start using python instead of shell for the update scripts. i wish i had in many places. it'd also work [better?] on windows. if you do decide to stick with shell, you should adhere to the shell style guide: https://google.github.io/styleguide/shell.xml https://codereview.chromium.org/2574033002/diff/280001/third_party/node/updat... File third_party/node/update_node_binaries.sh (right): https://codereview.chromium.org/2574033002/diff/280001/third_party/node/updat... third_party/node/update_node_binaries.sh:1: #!/bin/bash copyright https://codereview.chromium.org/2574033002/diff/280001/third_party/node/updat... File third_party/node/update_npm_deps.sh (right): https://codereview.chromium.org/2574033002/diff/280001/third_party/node/updat... third_party/node/update_npm_deps.sh:9: if [ -d "node_modules" ]; then this should probably be based on the location of the script, not the current working directory. if you run this from src/ bad things could happen or something
On 2017/01/10 at 22:09:13, dbeam wrote: > you might want to start using python instead of shell for the update scripts. i wish i had in many places. it'd also work [better?] on windows. > > if you do decide to stick with shell, you should adhere to the shell style guide: > https://google.github.io/styleguide/shell.xml > > https://codereview.chromium.org/2574033002/diff/280001/third_party/node/updat... > File third_party/node/update_node_binaries.sh (right): > > https://codereview.chromium.org/2574033002/diff/280001/third_party/node/updat... > third_party/node/update_node_binaries.sh:1: #!/bin/bash > copyright > > https://codereview.chromium.org/2574033002/diff/280001/third_party/node/updat... > File third_party/node/update_npm_deps.sh (right): > > https://codereview.chromium.org/2574033002/diff/280001/third_party/node/updat... > third_party/node/update_npm_deps.sh:9: if [ -d "node_modules" ]; then > this should probably be based on the location of the script, not the current working directory. > > if you run this from src/ bad things could happen or something I'll stick with bash for now (and address your bash related comments shortly). I can migrate to python as a follow up. My priority is to get MD Settings vulcanized, and these scripts are only useful to the few persons that will be rolling NPM/Node binaries, so not worth blocking Settings vulcanization on this.
Updated bash scripts to conform to the bash styleguide, as well as addressed issue with being able to execute the scripts from any folder. PTAL.
On 2017/01/11 19:08:11, dpapad wrote: > Updated bash scripts to conform to the bash styleguide, as well as addressed > issue with being able to execute the scripts from any folder. PTAL. https://google.github.io/styleguide/shell.xml#File_Extensions can we drop the .sh?
The CQ bit was checked by dpapad@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...
On 2017/01/11 at 19:09:37, dbeam wrote: > On 2017/01/11 19:08:11, dpapad wrote: > > Updated bash scripts to conform to the bash styleguide, as well as addressed > > issue with being able to execute the scripts from any folder. PTAL. > > https://google.github.io/styleguide/shell.xml#File_Extensions > > can we drop the .sh? Done.
i didn't review the bash with an insanely fine-toothed comb because i doubt there will be many running this lgtm https://codereview.chromium.org/2574033002/diff/320001/third_party/node/updat... File third_party/node/update_node_binaries (right): https://codereview.chromium.org/2574033002/diff/320001/third_party/node/updat... third_party/node/update_node_binaries:28: rm ${FOLDER}/${FILENAME} nit: in a couple cases you ask whether a directory or file exists before deleting it. why not just remove it unconditionally with -f? rm -f "${FOLDER}/${FILENAME}" https://codereview.chromium.org/2574033002/diff/320001/third_party/node/updat... third_party/node/update_node_binaries:103: sed -i "s/\(chromium-nodejs\/\)\([0-9]\|\.\)\+/\1${NODE_VERSION:1}/" ../../DEPS nit: can you use a different sed separator? like sed 's@blah@blee@' (/ -> @). we don't get as much saw toothing. https://codereview.chromium.org/2574033002/diff/320001/third_party/node/updat... third_party/node/update_node_binaries:103: sed -i "s/\(chromium-nodejs\/\)\([0-9]\|\.\)\+/\1${NODE_VERSION:1}/" ../../DEPS [0-9]\|\. could just be [0-9\.]
palmer@chromium.org changed reviewers: + palmer@chromium.org - security@chromium.org
Note for the future: It's best to send a note to security@chromium.org about the CL, but to CC specific security reviewers. I'll take a look at this today.
https://codereview.chromium.org/2574033002/diff/320001/third_party/node/updat... File third_party/node/update_node_binaries (right): https://codereview.chromium.org/2574033002/diff/320001/third_party/node/updat... third_party/node/update_node_binaries:28: rm ${FOLDER}/${FILENAME} > rm -f "${FOLDER}/${FILENAME}" It's crucial to use the quotes, by the way, to avoid shell injection attacks from file names. If a file were named "foo.txt; rm -rf ~" we'd be sad. :) It might be the case that these variables have values that we know are safe just from reading the code/statically; but I find it best to make it a habit to use the quotes so that you are always safe. https://codereview.chromium.org/2574033002/diff/320001/third_party/node/updat... third_party/node/update_node_binaries:30: wget -P ${FOLDER}/ ${URL} Same thing here: wget -P "${FOLDER}/" "${URL}" and throughout this script. If you want to know wayyyy more than you ever wanted to know, this is fun: http://www.dwheeler.com/essays/filenames-in-shell.html
https://codereview.chromium.org/2574033002/diff/320001/third_party/node/updat... File third_party/node/update_node_binaries (right): https://codereview.chromium.org/2574033002/diff/320001/third_party/node/updat... third_party/node/update_node_binaries:28: rm ${FOLDER}/${FILENAME} On 2017/01/11 20:53:51, palmer wrote: > > rm -f "${FOLDER}/${FILENAME}" > > It's crucial to use the quotes, by the way, to avoid shell injection attacks > from file names. If a file were named "foo.txt; rm -rf ~" we'd be sad. :) > > It might be the case that these variables have values that we know are safe just > from reading the code/statically; but I find it best to make it a habit to use > the quotes so that you are always safe. yeah, this is one of the things i didn't mention but always do myself. sorry!
https://codereview.chromium.org/2574033002/diff/320001/third_party/node/updat... File third_party/node/update_node_binaries (right): https://codereview.chromium.org/2574033002/diff/320001/third_party/node/updat... third_party/node/update_node_binaries:28: rm ${FOLDER}/${FILENAME} On 2017/01/11 at 20:17:50, Dan Beam wrote: > nit: in a couple cases you ask whether a directory or file exists before deleting it. why not just remove it unconditionally with -f? > > rm -f "${FOLDER}/${FILENAME}" Done. https://codereview.chromium.org/2574033002/diff/320001/third_party/node/updat... third_party/node/update_node_binaries:28: rm ${FOLDER}/${FILENAME} On 2017/01/11 at 20:58:58, Dan Beam wrote: > On 2017/01/11 20:53:51, palmer wrote: > > > rm -f "${FOLDER}/${FILENAME}" > > > > It's crucial to use the quotes, by the way, to avoid shell injection attacks > > from file names. If a file were named "foo.txt; rm -rf ~" we'd be sad. :) > > > > It might be the case that these variables have values that we know are safe just > > from reading the code/statically; but I find it best to make it a habit to use > > the quotes so that you are always safe. > > yeah, this is one of the things i didn't mention but always do myself. sorry! Done. https://codereview.chromium.org/2574033002/diff/320001/third_party/node/updat... third_party/node/update_node_binaries:30: wget -P ${FOLDER}/ ${URL} On 2017/01/11 at 20:53:51, palmer wrote: > Same thing here: > > wget -P "${FOLDER}/" "${URL}" > > and throughout this script. > > If you want to know wayyyy more than you ever wanted to know, this is fun: http://www.dwheeler.com/essays/filenames-in-shell.html Done. https://codereview.chromium.org/2574033002/diff/320001/third_party/node/updat... third_party/node/update_node_binaries:103: sed -i "s/\(chromium-nodejs\/\)\([0-9]\|\.\)\+/\1${NODE_VERSION:1}/" ../../DEPS On 2017/01/11 at 20:17:50, Dan Beam wrote: > [0-9]\|\. could just be [0-9\.] Done and done.
The CQ bit was checked by dpapad@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by dpapad@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.
Hi all, I haven't been able to get to this yet, my apologies! I will try to get to it by tomorrow.
@palmer ping. Is there anything else to address related to security?
https://codereview.chromium.org/2574033002/diff/380001/third_party/node/updat... File third_party/node/update_node_binaries (right): https://codereview.chromium.org/2574033002/diff/380001/third_party/node/updat... third_party/node/update_node_binaries:28: wget -P "${FOLDER}/" "${URL}" Might be good to check if wget fails, for whatever reason (including failure to check the nodejs.org HTTPS certificate). 1 thing you can do is set -e at the top of the script, to have it just bail out on the first error. If you don't want to do that you can just do the usual if ! wget ...; then exit 1 fi or whatever. https://codereview.chromium.org/2574033002/diff/380001/third_party/node/updat... third_party/node/update_node_binaries:41: echo "${sha1}" > "${FOLDER}/node-${SUFFIX}.tar.gz.sha1" Maybe check the checksum that Node distributes? E.g. https://nodejs.org/dist/latest-v6.x/SHASUMS256.txt. If gsutil needs SHA-1, it's ok to use that, but we should at least do the minimal check before assuming that the archive we got is good. It'd be even better if we could get the archive and the SHA-256s from different sources, in case 1 or the other gets compromised. (The real solution to this problem is Binary Transparency, but for now let's just dot our i's and cross our t's.) https://codereview.chromium.org/2574033002/diff/380001/third_party/node/updat... third_party/node/update_node_binaries:77: ####################################### Nit: I'd argue these comment blocks are unnecessary; the function names explain themselves well enough.
lgtm.
maruel@chromium.org changed reviewers: + maruel@chromium.org
https://codereview.chromium.org/2574033002/diff/380001/third_party/node/updat... File third_party/node/update_node_binaries (right): https://codereview.chromium.org/2574033002/diff/380001/third_party/node/updat... third_party/node/update_node_binaries:28: wget -P "${FOLDER}/" "${URL}" On 2017/01/13 01:02:27, palmer wrote: > Might be good to check if wget fails, for whatever reason (including failure to > check the http://nodejs.org HTTPS certificate). 1 thing you can do is > > set -e > > at the top of the script, to have it just bail out on the first error. If you > don't want to do that you can just do the usual > > if ! wget ...; then > exit 1 > fi > > or whatever. I highly recomment "set -eu". It's more annoying to write the script when you expect subcommands to normally exit non zero but it's much safer.
Addressed comments. PTAL. Also, after looking at the SHA256SUM file I realized that there was a gz binary available for Linux (instead of the xz I was using before), so I was able to merge update_linux and update_mac to a single method (now taking two parameters). https://codereview.chromium.org/2574033002/diff/380001/third_party/node/updat... File third_party/node/update_node_binaries (right): https://codereview.chromium.org/2574033002/diff/380001/third_party/node/updat... third_party/node/update_node_binaries:28: wget -P "${FOLDER}/" "${URL}" On 2017/01/13 at 02:25:04, M-A Ruel wrote: > On 2017/01/13 01:02:27, palmer wrote: > > Might be good to check if wget fails, for whatever reason (including failure to > > check the http://nodejs.org HTTPS certificate). 1 thing you can do is > > > > set -e > > > > at the top of the script, to have it just bail out on the first error. If you > > don't want to do that you can just do the usual > > > > if ! wget ...; then > > exit 1 > > fi > > > > or whatever. > > I highly recomment "set -eu". It's more annoying to write the script when you expect subcommands to normally exit non zero but it's much safer. Done. https://codereview.chromium.org/2574033002/diff/380001/third_party/node/updat... third_party/node/update_node_binaries:41: echo "${sha1}" > "${FOLDER}/node-${SUFFIX}.tar.gz.sha1" On 2017/01/13 at 01:02:28, palmer wrote: > Maybe check the checksum that Node distributes? E.g. https://nodejs.org/dist/latest-v6.x/SHASUMS256.txt. > > If gsutil needs SHA-1, it's ok to use that, but we should at least do the minimal check before assuming that the archive we got is good. > > It'd be even better if we could get the archive and the SHA-256s from different sources, in case 1 or the other gets compromised. (The real solution to this problem is Binary Transparency, but for now let's just dot our i's and cross our t's.) It's download_from_google_storage script that requires SHA1 (see https://cs.chromium.org/chromium/tools/depot_tools/download_from_google_stora... and all over that file). I added checks to ensure that the downloaded binaries match the SHA256 hashes. https://codereview.chromium.org/2574033002/diff/380001/third_party/node/updat... third_party/node/update_node_binaries:77: ####################################### On 2017/01/13 at 01:02:27, palmer wrote: > Nit: I'd argue these comment blocks are unnecessary; the function names explain themselves well enough. Removed.
still lgtm https://codereview.chromium.org/2574033002/diff/400001/third_party/node/updat... File third_party/node/update_node_binaries (right): https://codereview.chromium.org/2574033002/diff/400001/third_party/node/updat... third_party/node/update_node_binaries:70: echo $sha256_expected $sha256_actual debug statement?
https://codereview.chromium.org/2574033002/diff/400001/third_party/node/updat... File third_party/node/update_node_binaries (right): https://codereview.chromium.org/2574033002/diff/400001/third_party/node/updat... third_party/node/update_node_binaries:70: echo $sha256_expected $sha256_actual On 2017/01/13 at 03:34:12, Dan Beam wrote: > debug statement? Removed.
rs lgtm
LGTM % nits and quotes https://codereview.chromium.org/2574033002/diff/420001/third_party/node/updat... File third_party/node/update_node_binaries (right): https://codereview.chromium.org/2574033002/diff/420001/third_party/node/updat... third_party/node/update_node_binaries:31: sha256_expected="$(cat SHASUMS256.txt | grep ${FILENAME} | cut -d ' ' -f1)" You can shorten this to sha256_expected="$(grep ${FILENAME} SHASUMS256.txt | cut -d ' ' -f1)" I think it might be best to quote the file name, too; and the curly braces are unnecessary here: sha256_expected="$(grep "$FILENAME" SHASUMS256.txt | cut -d ' ' -f1)" https://codereview.chromium.org/2574033002/diff/420001/third_party/node/updat... third_party/node/update_node_binaries:32: local sha256_actual Nit: FWIW, you can combine the declaration and the definition: local sha256_actual=... https://codereview.chromium.org/2574033002/diff/420001/third_party/node/updat... third_party/node/update_node_binaries:33: sha256_actual="$(sha256sum ${FOLDER}/${FILENAME} | cut -d ' ' -f1)" Quotes here too, I think.
https://codereview.chromium.org/2574033002/diff/420001/third_party/node/updat... File third_party/node/update_node_binaries (right): https://codereview.chromium.org/2574033002/diff/420001/third_party/node/updat... third_party/node/update_node_binaries:31: sha256_expected="$(cat SHASUMS256.txt | grep ${FILENAME} | cut -d ' ' -f1)" On 2017/01/13 at 20:56:27, palmer wrote: > You can shorten this to > > sha256_expected="$(grep ${FILENAME} SHASUMS256.txt | cut -d ' ' -f1)" > > I think it might be best to quote the file name, too; and the curly braces are unnecessary here: > > sha256_expected="$(grep "$FILENAME" SHASUMS256.txt | cut -d ' ' -f1)" Done. https://codereview.chromium.org/2574033002/diff/420001/third_party/node/updat... third_party/node/update_node_binaries:32: local sha256_actual On 2017/01/13 at 20:56:27, palmer wrote: > Nit: FWIW, you can combine the declaration and the definition: > > local sha256_actual=... I am following the styleguide, which says: "Declare function-specific variables with local. Declaration and assignment should be on different lines." See, https://google.github.io/styleguide/shell.xml?showone=Use_Local_Variables#Use.... https://codereview.chromium.org/2574033002/diff/420001/third_party/node/updat... third_party/node/update_node_binaries:33: sha256_actual="$(sha256sum ${FOLDER}/${FILENAME} | cut -d ' ' -f1)" On 2017/01/13 at 20:56:28, palmer wrote: > Quotes here too, I think. Done.
Description was changed from ========== Downloading Node and NPM deps via gclient sync. This is necessary for WebUI to harness several Node based tools to speed up performance, as well as remove generated code that is currently checked in as source code. Discussion occurred at: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/H2IqgqwdUqs/yn_6_... BUG=673825 ========== to ========== Downloading Node and NPM deps via gclient sync. This is necessary for WebUI to harness several Node based tools to speed up performance, as well as remove generated code that is currently checked in as source code. Discussion occurred at: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/H2IqgqwdUqs/yn_6_... BUG=673825 ==========
dbeam@chromium.org changed reviewers: - scottchen@chromium.org
FYI open source licensing review completed.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, dbeam@chromium.org, palmer@chromium.org, maruel@chromium.org Link to the patchset: https://codereview.chromium.org/2574033002/#ps440001 (title: "Nits.")
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_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 dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org, dpranke@chromium.org, dbeam@chromium.org, maruel@chromium.org Link to the patchset: https://codereview.chromium.org/2574033002/#ps460001 (title: "Add missing sha1 file.")
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": 460001, "attempt_start_ts": 1484358936935960,
"parent_rev": "85990128f90e66efb2d1a83caeca578ab93c275e", "commit_rev":
"cbee6ab37edb7df0d8d5354fd2af29e968f33e94"}
Message was sent while issue was closed.
Description was changed from ========== Downloading Node and NPM deps via gclient sync. This is necessary for WebUI to harness several Node based tools to speed up performance, as well as remove generated code that is currently checked in as source code. Discussion occurred at: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/H2IqgqwdUqs/yn_6_... BUG=673825 ========== to ========== Downloading Node and NPM deps via gclient sync. This is necessary for WebUI to harness several Node based tools to speed up performance, as well as remove generated code that is currently checked in as source code. Discussion occurred at: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/H2IqgqwdUqs/yn_6_... BUG=673825 Review-Url: https://codereview.chromium.org/2574033002 Cr-Commit-Position: refs/heads/master@{#443777} Committed: https://chromium.googlesource.com/chromium/src/+/cbee6ab37edb7df0d8d5354fd2af... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:460001) as https://chromium.googlesource.com/chromium/src/+/cbee6ab37edb7df0d8d5354fd2af...
Message was sent while issue was closed.
wez@chromium.org changed reviewers: + sky@chromium.org
Message was sent while issue was closed.
Hallo sky@chromium.org! Due to a depot_tools patch which mistakenly removed the OWNERS check for non-source files (see crbug.com/684270), the following files landed in this CL and need a retrospective review from you: third_party/node/LICENSE third_party/node/OWNERS third_party/node/README.chromium third_party/node/linux/node-linux-x64.tar.gz.sha1 third_party/node/mac/node-darwin-x64.tar.gz.sha1 third_party/node/node_modules.tar.gz.sha1 third_party/node/npm_exclude.txt third_party/node/package.json third_party/node/update_node_binaries third_party/node/update_npm_deps third_party/node/win/node.exe.sha1 Thanks, Wez |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
