Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
50423003:
Adds a flag "use_instrumented_libraries" and corresponding target with 2 simple libs (Closed)
|
Created:
7 years, 1 month ago by alextaran1 Modified:
7 years, 1 month ago CC:
chromium-reviews Base URL:
https://src.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdds a flag "use_instrumented_libraries" and corresponding target with 2 simple libraries
BUG=313751
R=glider@chromium.org,thakis@chromium.org
TBR=cpu@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234498
Patch Set 1 #
Total comments: 43
Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 35
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : Adds a flag "use_instrumented_libraries" and corresponding target with 2 simple libraries #Patch Set 9 : Adds a flag "use_instrumented_libraries" and corresponding target with 2 simple libraries #Patch Set 10 : #Patch Set 11 : #
Messages
Total messages: 37 (0 generated)
Please take a look
https://codereview.chromium.org/50423003/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/50423003/diff/1/build/common.gypi#newcode312 build/common.gypi:312: # Enable use pre-built instrumented system libs by sanitizer instead of standard libs Please make sure you stay below the 80-column limit. Also, this comment should end with a period. https://codereview.chromium.org/50423003/diff/1/build/common.gypi#newcode312 build/common.gypi:312: # Enable use pre-built instrumented system libs by sanitizer instead of standard libs How about "Use the libraries instrumented by one of the sanitizers instead of the standard system libraries"? https://codereview.chromium.org/50423003/diff/1/build/common.gypi#newcode3382 build/common.gypi:3382: '-Wl,-R,\\$$ORIGIN/instrumented_libraries/asan/lib/:\\$$ORIGIN/instrumented_libraries/asan/usr/lib/x86_64-linux-gnu/', Pls add a comment for these flags. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... File third_party/instrumented_libraries/download_build_install (right): https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:1: #!/bin/bash A Bash script still needs the copyright text. For example, take a look at tools/clang/scripts/update.sh. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:3: # Downloads, builds (with instrumentation) and installs shared libraries Period at the end of the comment. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:5: export CFLAGS="-fsanitize=address -g -fPIC -w" We must potentially support other -fsanitize= flags. You can add a "TODO(alextaran)" about that right now. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:12: # Colored print Doesn't ninja swallow your colored print? https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:28: install_dir=$(pwd)/asan Again, we must support other tools in the future - add a TODO https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:37: echo_green "Installing dependencies for: $OPTARG" Please check with http://google-styleguide.googlecode.com/svn/trunk/shell.xml#Variable_expansion for details when the variable names must be brace-quoted and fix all the vars according to that. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:46: -i - sets relative install_dir." I suggest to pass only the parameters the script cannot infer itself. I.e. the tool name, <(PRODUCT_DIR), <(INTERMEDIATE_DIR), library name. Let the script make $intermediate_dir and $install_dir from these parameters instead of passing '<(PRODUCT_DIR)/instrumented_libraries/asan' for each GYP target. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:74: | cut -d " " -f 2) I think there should be one more space before the pipe (4 instead of 3) https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:85: mkdir -p $intermediate_dir/$library Make sure to quote the variable names. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:89: cd $(ls -F |grep \/$) Quite a cryptic line. Pleas add a comment. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:96: make install You can install in parallel, too: make $MAKEJOBS install. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:100: touch $install_dir/$library.txt Is this file actually used anywhere now? https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... File third_party/instrumented_libraries/fix_rpaths (right): https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/fix_rpaths:3: # Changes all RPATHs from XORIGIN to $ORIGIN "all RPATHs in a given directory" https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/fix_rpaths:5: # Fixes rpath from XORIGIN to $ORIGIN in a single file $1 Period at the end of the comment here and everywhere else. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/fix_rpaths:12: fix_rpath $i > /dev/null Why suppress the output? Does chrpath print too much? https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... File third_party/instrumented_libraries/instrumented_libraries.gyp (right): https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/instrumented_libraries.gyp:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. It's 2013 already :) https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/instrumented_libraries.gyp:33: 'dependencies=': [ No need to have an empty dependency list, please delete it. (also, what's 'dependencies='?) https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/instrumented_libraries.gyp:49: 'target_name': 'libxau6', There's too much boilerplate for each library. I wonder if we can reduce it (you don't have to do anything about it right now).
https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... File third_party/instrumented_libraries/instrumented_libraries.gyp (right): https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/instrumented_libraries.gyp:33: 'dependencies=': [ On 2013/10/29 13:36:11, Alexander Potapenko wrote: > No need to have an empty dependency list, please delete it. > (also, what's 'dependencies='?) Please disregard.
please take another look https://codereview.chromium.org/50423003/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/50423003/diff/1/build/common.gypi#newcode312 build/common.gypi:312: # Enable use pre-built instrumented system libs by sanitizer instead of standard libs On 2013/10/29 13:36:11, Alexander Potapenko wrote: > How about "Use the libraries instrumented by one of the sanitizers instead of > the standard system libraries"? Thanks, that's much better https://codereview.chromium.org/50423003/diff/1/build/common.gypi#newcode312 build/common.gypi:312: # Enable use pre-built instrumented system libs by sanitizer instead of standard libs On 2013/10/29 13:36:11, Alexander Potapenko wrote: > Please make sure you stay below the 80-column limit. > Also, this comment should end with a period. There are too many lines in common.gypi which have length >80 symbols https://codereview.chromium.org/50423003/diff/1/build/common.gypi#newcode3382 build/common.gypi:3382: '-Wl,-R,\\$$ORIGIN/instrumented_libraries/asan/lib/:\\$$ORIGIN/instrumented_libraries/asan/usr/lib/x86_64-linux-gnu/', On 2013/10/29 13:36:11, Alexander Potapenko wrote: > Pls add a comment for these flags. Done. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... File third_party/instrumented_libraries/download_build_install (right): https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:1: #!/bin/bash On 2013/10/29 13:36:11, Alexander Potapenko wrote: > A Bash script still needs the copyright text. For example, take a look at > tools/clang/scripts/update.sh. Done. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:3: # Downloads, builds (with instrumentation) and installs shared libraries On 2013/10/29 13:36:11, Alexander Potapenko wrote: > Period at the end of the comment. Done. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:5: export CFLAGS="-fsanitize=address -g -fPIC -w" On 2013/10/29 13:36:11, Alexander Potapenko wrote: > We must potentially support other -fsanitize= flags. > You can add a "TODO(alextaran)" about that right now. Added "sanitizer" flag to script https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:12: # Colored print On 2013/10/29 13:36:11, Alexander Potapenko wrote: > Doesn't ninja swallow your colored print? It works good :) https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:28: install_dir=$(pwd)/asan On 2013/10/29 13:36:11, Alexander Potapenko wrote: > Again, we must support other tools in the future - add a TODO Added a "sanitizer" flag to script https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:37: echo_green "Installing dependencies for: $OPTARG" On 2013/10/29 13:36:11, Alexander Potapenko wrote: > Please check with > http://google-styleguide.googlecode.com/svn/trunk/shell.xml#Variable_expansion > for details when the variable names must be brace-quoted and fix all the vars > according to that. Done. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:46: -i - sets relative install_dir." On 2013/10/29 13:36:11, Alexander Potapenko wrote: > I suggest to pass only the parameters the script cannot infer itself. I.e. the > tool name, <(PRODUCT_DIR), <(INTERMEDIATE_DIR), library name. > Let the script make $intermediate_dir and $install_dir from these parameters > instead of passing '<(PRODUCT_DIR)/instrumented_libraries/asan' for each GYP > target. Done. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:74: | cut -d " " -f 2) It has actually 2 spaces according to http://google-styleguide.googlecode.com/svn/trunk/shell.xml?showone=Pipelines... On 2013/10/29 13:36:11, Alexander Potapenko wrote: > I think there should be one more space before the pipe (4 instead of 3) https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:85: mkdir -p $intermediate_dir/$library On 2013/10/29 13:36:11, Alexander Potapenko wrote: > Make sure to quote the variable names. Done. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:89: cd $(ls -F |grep \/$) On 2013/10/29 13:36:11, Alexander Potapenko wrote: > Quite a cryptic line. Pleas add a comment. Done. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:96: make install On 2013/10/29 13:36:11, Alexander Potapenko wrote: > You can install in parallel, too: make $MAKEJOBS install. Done. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/download_build_install:100: touch $install_dir/$library.txt It's used by GYP for valid incremental build. I will add a comment for this. On 2013/10/29 13:36:11, Alexander Potapenko wrote: > Is this file actually used anywhere now? https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... File third_party/instrumented_libraries/fix_rpaths (right): https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/fix_rpaths:3: # Changes all RPATHs from XORIGIN to $ORIGIN On 2013/10/29 13:36:11, Alexander Potapenko wrote: > "all RPATHs in a given directory" Done. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/fix_rpaths:5: # Fixes rpath from XORIGIN to $ORIGIN in a single file $1 On 2013/10/29 13:36:11, Alexander Potapenko wrote: > Period at the end of the comment here and everywhere else. Done. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/fix_rpaths:12: fix_rpath $i > /dev/null On 2013/10/29 13:36:11, Alexander Potapenko wrote: > Why suppress the output? Does chrpath print too much? It prints 2 lines for each file: old rpath and new rpath. And it doesn't have command-line option for silence. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... File third_party/instrumented_libraries/instrumented_libraries.gyp (right): https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/instrumented_libraries.gyp:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/10/29 13:36:11, Alexander Potapenko wrote: > It's 2013 already :) Done. https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/instrumented_libraries.gyp:33: 'dependencies=': [ On 2013/10/29 13:57:04, Alexander Potapenko wrote: > On 2013/10/29 13:36:11, Alexander Potapenko wrote: > > No need to have an empty dependency list, please delete it. > > (also, what's 'dependencies='?) > > Please disregard. Disregarded. It's needed to remove cyclic dependency. This simple string removes all dependencies, after that this target depends on nothing (as it should be). https://codereview.chromium.org/50423003/diff/1/third_party/instrumented_libr... third_party/instrumented_libraries/instrumented_libraries.gyp:49: 'target_name': 'libxau6', On 2013/10/29 13:36:11, Alexander Potapenko wrote: > There's too much boilerplate for each library. I wonder if we can reduce it (you > don't have to do anything about it right now). Done nothing. In future, we'll pass different build options for different libraries through this gyp-file. This boilerplate seems like minimal amount of code for building a typical library. Probably, we can get rid of some parts (or find more appropriate solution), but it's not obvious for me now.
Looks good in general. Nico, can you please take a look? https://codereview.chromium.org/50423003/diff/100001/third_party/instrumented... File third_party/instrumented_libraries/fix_rpaths (right): https://codereview.chromium.org/50423003/diff/100001/third_party/instrumented... third_party/instrumented_libraries/fix_rpaths:10: chrpath -r $(chrpath $1 | cut -d " " -f 2 | sed s/XORIGIN/\$ORIGIN/g \ I suggest to redirect stdout to /dev/null right here instead of doing that in the loop below. https://codereview.chromium.org/50423003/diff/100001/third_party/instrumented... File third_party/instrumented_libraries/instrumented_libraries.gyp (right): https://codereview.chromium.org/50423003/diff/100001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:21: 'fix_rpaths' please add a comma after the list item https://codereview.chromium.org/50423003/diff/100001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:24: '<(PRODUCT_DIR)/instrumented_libraries/asan/rpaths.fixed.txt' please add a comma after the list item https://codereview.chromium.org/50423003/diff/100001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:42: '<(PRODUCT_DIR)/instrumented_libraries/asan/<(_target_name).txt' comma here https://codereview.chromium.org/50423003/diff/100001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:60: '<(PRODUCT_DIR)/instrumented_libraries/asan/<(_target_name).txt' comma here
Done small fix https://codereview.chromium.org/50423003/diff/100001/third_party/instrumented... File third_party/instrumented_libraries/fix_rpaths (right): https://codereview.chromium.org/50423003/diff/100001/third_party/instrumented... third_party/instrumented_libraries/fix_rpaths:10: chrpath -r $(chrpath $1 | cut -d " " -f 2 | sed s/XORIGIN/\$ORIGIN/g \ On 2013/10/30 11:46:27, Alexander Potapenko wrote: > I suggest to redirect stdout to /dev/null right here instead of doing that in > the loop below. Done. https://codereview.chromium.org/50423003/diff/100001/third_party/instrumented... File third_party/instrumented_libraries/instrumented_libraries.gyp (right): https://codereview.chromium.org/50423003/diff/100001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:21: 'fix_rpaths' On 2013/10/30 11:46:27, Alexander Potapenko wrote: > please add a comma after the list item Done. https://codereview.chromium.org/50423003/diff/100001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:24: '<(PRODUCT_DIR)/instrumented_libraries/asan/rpaths.fixed.txt' On 2013/10/30 11:46:27, Alexander Potapenko wrote: > please add a comma after the list item Done. https://codereview.chromium.org/50423003/diff/100001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:42: '<(PRODUCT_DIR)/instrumented_libraries/asan/<(_target_name).txt' On 2013/10/30 11:46:27, Alexander Potapenko wrote: > comma here Done. https://codereview.chromium.org/50423003/diff/100001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:60: '<(PRODUCT_DIR)/instrumented_libraries/asan/<(_target_name).txt' On 2013/10/30 11:46:27, Alexander Potapenko wrote: > comma here Done.
Sorry about the many comments. I'm guessing this is for the "have instrumented system libraries" project? Please file a tracking bug for that and add a BUG= line. https://codereview.chromium.org/50423003/diff/180001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/50423003/diff/180001/build/common.gypi#newcod... build/common.gypi:3375: 'dependencies': [ Why do you always need this but the rpath bit only for asan? https://codereview.chromium.org/50423003/diff/180001/build/common.gypi#newcod... build/common.gypi:3384: '-Wl,-R,\\$$ORIGIN/instrumented_libraries/asan/lib/:\\$$ORIGIN/instrumented_libraries/asan/usr/lib/x86_64-linux-gnu/', Just one backslash in front of $ is enough (and consistent with how we spell it in other place) https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... File third_party/instrumented_libraries/download_build_install (right): https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/download_build_install:24: product_dir=$(pwd) Scripts shouldn't depend on the pwd https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/download_build_install:51: product_dir="$(pwd)/${OPTARG}" Does this need to be configurable? Can you just hardcode $(dirname "${0}")/../instrumented_libraries_build or similar? https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/download_build_install:83: if [[ -z "${sanitizer_type}" ]]; then Can this be python instead? https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/download_build_install:108: apt-get source ${library} 2>&1 How does this work on non-apt systems? https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... File third_party/instrumented_libraries/fix_rpaths (right): https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/fix_rpaths:6: # Changes all RPATHs in a given directory from XORIGIN to $ORIGIN Why is this needed? Can't you set them to the right thing in the first place? https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/fix_rpaths:19: # This file is used by GYP as 'output' to mark that RPATHs are alreaty fixed typo alreaty https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/fix_rpaths:20: # while making incremental build. "for incremental builds" https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... File third_party/instrumented_libraries/instrumented_libraries.gyp (right): https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:11: 'prune_self_dependency': 1, Do you need this? https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:24: '<(PRODUCT_DIR)/instrumented_libraries/asan/rpaths.fixed.txt', build outputs generally shouldn't be written into the source directory. Maybe use <(INTERMEDIATE_DIR) instead? https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:31: 'target_name': 'libpng12-0', Is the idea to have one target like this for every system library?
https://codereview.chromium.org/50423003/diff/180001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/50423003/diff/180001/build/common.gypi#newcod... build/common.gypi:3375: 'dependencies': [ On 2013/10/30 14:47:02, Nico wrote: > Why do you always need this but the rpath bit only for asan? We are going to have different instrumentation types (asan, msan, tsan). Later we can check asan==1 or tsan==1 in instrumented_libraries.gyp to build with right instrumentation. https://codereview.chromium.org/50423003/diff/180001/build/common.gypi#newcod... build/common.gypi:3384: '-Wl,-R,\\$$ORIGIN/instrumented_libraries/asan/lib/:\\$$ORIGIN/instrumented_libraries/asan/usr/lib/x86_64-linux-gnu/', On 2013/10/30 14:47:02, Nico wrote: > Just one backslash in front of $ is enough (and consistent with how we spell it > in other place) Done. https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... File third_party/instrumented_libraries/download_build_install (right): https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/download_build_install:24: product_dir=$(pwd) On 2013/10/30 14:47:02, Nico wrote: > Scripts shouldn't depend on the pwd The library should be installed into <(product_dir)/some_folder_for_libs, "product_dir" is relative from this script. We need to call ./configure with parameter --prefix=path_where_install_out_library ./configure requires absolute path (http://gcc.gnu.org/ml/gcc/2006-05/msg00646.html). Any ideas how to get rid of $(pwd) completely? https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/download_build_install:51: product_dir="$(pwd)/${OPTARG}" On 2013/10/30 14:47:02, Nico wrote: > Does this need to be configurable? Can you just hardcode $(dirname > "${0}")/../instrumented_libraries_build or similar? product_dir should be a path to chrome binary (or other binaries). How can we know where libraries should be put? ../../out/Release or ../../out/Debug ? https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/download_build_install:83: if [[ -z "${sanitizer_type}" ]]; then On 2013/10/30 14:47:02, Nico wrote: > Can this be python instead? Not sure what do you mean here.. https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/download_build_install:108: apt-get source ${library} 2>&1 On 2013/10/30 14:47:02, Nico wrote: > How does this work on non-apt systems? This will not work. Does anybody want to build chrome with instrumented libraries on non-apt system? Hardcoding URL for each library sources here and using git or svn? Does it look like an appropriate solution. https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... File third_party/instrumented_libraries/fix_rpaths (right): https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/fix_rpaths:6: # Changes all RPATHs in a given directory from XORIGIN to $ORIGIN On 2013/10/30 14:47:02, Nico wrote: > Why is this needed? Can't you set them to the right thing in the first place? No I can't. We need to export ..$ORIGIN.. to LDFLAGS in "download_build_install" script, but this string contains a dollar sign "$". "configure" uses this variable and writes to Makefile. make runs commands from Makefile. And each of tries to use substitution rules. Different libraries use different structure of build files and LDFLAGS pass through substitution rules differently. Moreover, Makefile uses $$ to escape $, and bash-scripts use the other. "Thanks" to autotools. https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/fix_rpaths:19: # This file is used by GYP as 'output' to mark that RPATHs are alreaty fixed On 2013/10/30 14:47:02, Nico wrote: > typo alreaty Done. https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/fix_rpaths:20: # while making incremental build. On 2013/10/30 14:47:02, Nico wrote: > "for incremental builds" Done. https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... File third_party/instrumented_libraries/instrumented_libraries.gyp (right): https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:11: 'prune_self_dependency': 1, On 2013/10/30 14:47:02, Nico wrote: > Do you need this? cyclic dependency otherwise. https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:24: '<(PRODUCT_DIR)/instrumented_libraries/asan/rpaths.fixed.txt', On 2013/10/30 14:47:02, Nico wrote: > build outputs generally shouldn't be written into the source directory. Maybe > use <(INTERMEDIATE_DIR) instead? Build outputs are actually "*.so" files and many other files after running "make install". It depends on library, but GYP should determine: does it have the library built or not. This file is used only to detect that it's not necessary to download library once again and build it. https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:31: 'target_name': 'libpng12-0', On 2013/10/30 14:47:02, Nico wrote: > Is the idea to have one target like this for every system library? Yes.
https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... File third_party/instrumented_libraries/download_build_install (right): https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/download_build_install:24: product_dir=$(pwd) As Nico suggested below, $(dirname "${0}") is bettern than $(pwd). https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/download_build_install:108: apt-get source ${library} 2>&1 On 2013/10/30 14:47:02, Nico wrote: > How does this work on non-apt systems? Apt systems (namely Ubuntu) are first-class citizens in Chromium, while others aren't. Our primary clients are ClusterFuzz and the buildbots, so it's ok to assume apt is available. Most of the developers are running Ubuntu as well. https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... File third_party/instrumented_libraries/fix_rpaths (right): https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/fix_rpaths:6: # Changes all RPATHs in a given directory from XORIGIN to $ORIGIN Please explain this in a comment in download_build_install and here (ok to just write "see the comment in download_build_install"). https://codereview.chromium.org/50423003/diff/180002/third_party/instrumented... File third_party/instrumented_libraries/download_build_install (right): https://codereview.chromium.org/50423003/diff/180002/third_party/instrumented... third_party/instrumented_libraries/download_build_install:1: #!/bin/bash I prefer shell scripts to have .sh extensions, although YMMV.
https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... File third_party/instrumented_libraries/download_build_install (right): https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/download_build_install:24: product_dir=$(pwd) On 2013/10/31 12:18:16, Alexander Potapenko wrote: > As Nico suggested below, $(dirname "${0}") is bettern than $(pwd). Added cd $(dirname "${0}") in the beginning. After that, $(pwd) should give us a valid absolute path to pass it to configure script ($(dirname "${0}") doesn't give absolute path) https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... File third_party/instrumented_libraries/fix_rpaths (right): https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/fix_rpaths:6: # Changes all RPATHs in a given directory from XORIGIN to $ORIGIN On 2013/10/31 12:18:16, Alexander Potapenko wrote: > Please explain this in a comment in download_build_install and here (ok to just > write "see the comment in download_build_install"). Done. https://codereview.chromium.org/50423003/diff/180002/third_party/instrumented... File third_party/instrumented_libraries/download_build_install (right): https://codereview.chromium.org/50423003/diff/180002/third_party/instrumented... third_party/instrumented_libraries/download_build_install:1: #!/bin/bash On 2013/10/31 12:18:17, Alexander Potapenko wrote: > I prefer shell scripts to have .sh extensions, although YMMV. Removing extensions from shell scripts is according to shell codestyle, but scripts in chromium repository have extensions. So you're right, let's be consistent with the repository.
Sorry for the delay. I think this is mostly fine, modulo a few tweaks minor tweaks. I'd like to see a detailed discussion somewhere (on the bug maybe) why downloading binaries is preferable over compiling source files. (Advantages: probably faster, etc. Downsides: abi concerns, etc) I do believe you that binaries are the right choice (assuming you guys own updating them after abi changes, like discussed on the doc), I'd just like to see the reasoning why. https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... File third_party/instrumented_libraries/download_build_install (right): https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/download_build_install:83: if [[ -z "${sanitizer_type}" ]]; then On 2013/10/30 16:08:54, alextaran1 wrote: > On 2013/10/30 14:47:02, Nico wrote: > > Can this be python instead? > > Not sure what do you mean here.. Could this whole script be a python script instead of a bash script? bash should only be used for very short scripts, this script is already somewhat long, and they always grow over time. https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... File third_party/instrumented_libraries/fix_rpaths (right): https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/fix_rpaths:3: # Use of this source code is governed by a BSD-style license that can be (this one is ok as bash script for now for example, since it's short and easy to rewrite in python when it starts growing) https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... File third_party/instrumented_libraries/instrumented_libraries.gyp (right): https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:24: '<(PRODUCT_DIR)/instrumented_libraries/asan/rpaths.fixed.txt', On 2013/10/30 16:08:54, alextaran1 wrote: > On 2013/10/30 14:47:02, Nico wrote: > > build outputs generally shouldn't be written into the source directory. Maybe > > use <(INTERMEDIATE_DIR) instead? > > Build outputs are actually "*.so" files and many other files after running "make > install". It depends on library, but GYP should determine: does it have the > library built or not. This file is used only to detect that it's not necessary > to download library once again and build it. Sorry, <(PRODUCT_DIR) evaluates to a build directory. I'm not sure what I was thinking here, ignore this comment. https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:31: 'target_name': 'libpng12-0', On 2013/10/30 16:08:54, alextaran1 wrote: > On 2013/10/30 14:47:02, Nico wrote: > > Is the idea to have one target like this for every system library? > > Yes. Hm. What's the advantage of downloading binaries over checking in the source and building these products then? https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:44: 'action': ['./download_build_install', '-i', '<(PRODUCT_DIR)', '-l', '<(_target_name)', '-m', '<(INTERMEDIATE_DIR)', '-s', 'asan'], If you want to keep the download action, please move everything that's shared between these wrapper targets into a gypi file and include that into every target. It sounds like there are going to be many of them, so each of them should be short.
On 2013/11/02 00:05:02, Nico wrote: > Sorry for the delay. > > I think this is mostly fine, modulo a few tweaks minor tweaks. > > I'd like to see a detailed discussion somewhere (on the bug maybe) why > downloading binaries is preferable over compiling source files. (Advantages: > probably faster, etc. Downsides: abi concerns, etc) I do believe you that > binaries are the right choice (assuming you guys own updating them after abi This whole CL is about building the libraries from sources, not downloading them, just to avoid supporting the binaries and all the abi hassle. Should we have a detailed discussion why this is preferable instead? :)
https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... File third_party/instrumented_libraries/download_build_install (right): https://codereview.chromium.org/50423003/diff/180001/third_party/instrumented... third_party/instrumented_libraries/download_build_install:83: if [[ -z "${sanitizer_type}" ]]; then > Could this whole script be a python script instead of a bash script? bash should > only be used for very short scripts, this script is already somewhat long, and > they always grow over time. I support this, but maybe we can rewrite the script in a follow-up CL?
Moved common parts of instrumented_libraries.gypi to separate file. https://codereview.chromium.org/50423003/diff/570001/third_party/instrumented... File third_party/instrumented_libraries/instrumented_libraries.gyp (right): https://codereview.chromium.org/50423003/diff/570001/third_party/instrumented... third_party/instrumented_libraries/instrumented_libraries.gyp:32: 'dependencies=': [], Unfortunately, I have to leave 'dependencies=': [] here for each target. If I put it into standard_instrumented_library_target.gypi, the cyclic dependency will occur.
lgtm Sorry for the delay! https://codereview.chromium.org/50423003/diff/570001/third_party/instrumented... File third_party/instrumented_libraries/standard_instrumented_library_target.gypi (right): https://codereview.chromium.org/50423003/diff/570001/third_party/instrumented... third_party/instrumented_libraries/standard_instrumented_library_target.gypi:5: # This file is meant to be included into a target for instrumented dynamic libraries (nit: wrap comment at 80 cols maybe)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alextaran@chromium.org/50423003/630001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
We should've put this stuff into tools/, not third_party/. Please fix. On Nov 8, 2013 3:39 PM, <commit-bot@chromium.org> wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/tryserver.chromium/ > buildstatus?builder=chromium_presubmit&number=35208 > > https://codereview.chromium.org/50423003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Adds a flag "use_instrumented_libraries" and corresponding target with 2 simple libraries BUG=313751 R=glider@chromium.org,thakis@chromium.org
On 2013/11/08 12:54:18, alextaran1 wrote: > Adds a flag "use_instrumented_libraries" and corresponding target with 2 simple > libraries > > BUG=313751 > mailto:R=glider@chromium.org,thakis@chromium.org Please also fix the occurrences of third_party.
On 2013/11/08 12:40:09, Alexander Potapenko wrote: > We should've put this stuff into tools/, not third_party/. Please fix. Why?
On 2013/11/09 06:31:09, Nico (ooo until Nov 12) wrote: > On 2013/11/08 12:40:09, Alexander Potapenko wrote: > > We should've put this stuff into tools/, not third_party/. Please fix. > > Why? Because we don't keep any third party code in this dir. Do you have a reason to keep the scripts in third_party?
On Fri, Nov 8, 2013 at 10:41 PM, <glider@chromium.org> wrote: > On 2013/11/09 06:31:09, Nico (ooo until Nov 12) wrote: > >> On 2013/11/08 12:40:09, Alexander Potapenko wrote: >> > We should've put this stuff into tools/, not third_party/. Please fix. >> > > Why? >> > > Because we don't keep any third party code in this dir. Do you have a > reason to > keep the scripts in third_party? They're pulling for third-party code. (I don't feel strongly, but third_party looked good to me.) To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I thought we may run into issues with various scripts treating third_party differently (e.g. the presubmit fails, presumably because of the missing license files, haven't had a close look yet). On Nov 9, 2013 10:53 AM, "Nico Weber" <thakis@chromium.org> wrote: > On Fri, Nov 8, 2013 at 10:41 PM, <glider@chromium.org> wrote: > >> On 2013/11/09 06:31:09, Nico (ooo until Nov 12) wrote: >> >>> On 2013/11/08 12:40:09, Alexander Potapenko wrote: >>> > We should've put this stuff into tools/, not third_party/. Please fix. >>> >> >> Why? >>> >> >> Because we don't keep any third party code in this dir. Do you have a >> reason to >> keep the scripts in third_party? > > > They're pulling for third-party code. (I don't feel strongly, but > third_party looked good to me.) > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Alex, the CQ is failing anyway, let us decide tomorrow (maybe third_party is ok) On Nov 9, 2013 3:46 PM, "Alexander Potapenko" <glider@chromium.org> wrote: > I thought we may run into issues with various scripts treating third_party > differently (e.g. the presubmit fails, presumably because of the missing > license files, haven't had a close look yet). > On Nov 9, 2013 10:53 AM, "Nico Weber" <thakis@chromium.org> wrote: > >> On Fri, Nov 8, 2013 at 10:41 PM, <glider@chromium.org> wrote: >> >>> On 2013/11/09 06:31:09, Nico (ooo until Nov 12) wrote: >>> >>>> On 2013/11/08 12:40:09, Alexander Potapenko wrote: >>>> > We should've put this stuff into tools/, not third_party/. Please fix. >>>> >>> >>> Why? >>>> >>> >>> Because we don't keep any third party code in this dir. Do you have a >>> reason to >>> keep the scripts in third_party? >> >> >> They're pulling for third-party code. (I don't feel strongly, but >> third_party looked good to me.) >> >> To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Sat, Nov 9, 2013 at 3:46 AM, Alexander Potapenko <glider@chromium.org>wrote: > I thought we may run into issues with various scripts treating third_party > differently (e.g. the presubmit fails, presumably because of the missing > license files, haven't had a close look yet). > If it makes things easier for some technical reason (e.g presubmit), tools/ is fine too. > On Nov 9, 2013 10:53 AM, "Nico Weber" <thakis@chromium.org> wrote: > >> On Fri, Nov 8, 2013 at 10:41 PM, <glider@chromium.org> wrote: >> >>> On 2013/11/09 06:31:09, Nico (ooo until Nov 12) wrote: >>> >>>> On 2013/11/08 12:40:09, Alexander Potapenko wrote: >>>> > We should've put this stuff into tools/, not third_party/. Please fix. >>>> >>> >>> Why? >>>> >>> >>> Because we don't keep any third party code in this dir. Do you have a >>> reason to >>> keep the scripts in third_party? >> >> >> They're pulling for third-party code. (I don't feel strongly, but >> third_party looked good to me.) >> >> To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Adds a flag "use_instrumented_libraries" and corresponding target with 2 simple libraries BUG=313751 R=glider@chromium.org,thakis@chromium.org,cpu@chromium.org
Please take a look (added: README.chromium)
Still LGTM. Carlos, we need your approval for third_party. Please note that there's no actual third party code being committed, only Chromium-owned scripts.
F:\src\t0\src\third_party>more OWNERS # Owner approval for 3rd party is only required for # adding new libraries. For changes to existing code # simply TBR= one of people below. brettw@chromium.org cpu@chromium.org darin@chromium.org
Well, this was adding new entities to third_party, so we thought it's equivalent to adding libraries. Sorry for disturbing. On Nov 11, 2013 10:34 PM, <cpu@chromium.org> wrote: > > F:\src\t0\src\third_party>more OWNERS > # Owner approval for 3rd party is only required for > # adding new libraries. For changes to existing code > # simply TBR= one of people below. > > brettw@chromium.org > cpu@chromium.org > darin@chromium.org > > https://codereview.chromium.org/50423003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alextaran@chromium.org/50423003/1260001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
On 2013/11/12 08:36:33, I haz the power (commit-bot) wrote: > Step "update" is always a major failure. > Look at the try server FAQ for more details. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Removed svn:executable flags from scripts
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alextaran@chromium.org/50423003/640006
Message was sent while issue was closed.
Change committed as 234498
On Tue, Nov 5, 2013 at 5:25 AM, <glider@chromium.org> wrote: > On 2013/11/02 00:05:02, Nico wrote: > >> Sorry for the delay. >> > > I think this is mostly fine, modulo a few tweaks minor tweaks. >> > > I'd like to see a detailed discussion somewhere (on the bug maybe) why >> downloading binaries is preferable over compiling source files. >> (Advantages: >> probably faster, etc. Downsides: abi concerns, etc) I do believe you that >> binaries are the right choice (assuming you guys own updating them after >> abi >> > > This whole CL is about building the libraries from sources, not downloading > them, just to avoid supporting the binaries and all the abi hassle. > Should we have a detailed discussion why this is preferable instead? :) > I guess what confused me here is that the source is pulled via apt, instead of via deps (which is what we usually use to pull source). Since apt is causing some issues, maybe using deps should be reevaluated? (Just a suggestion. I generally like uniformity in our infrastructure, but you can judge the tradeoffs here better than I.) > > https://codereview.chromium.org/50423003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
