Rewrited build script for instrumented libraries from bash to python

Rewrote build script for instrumented libraries in python

R=glider@chromium.org
TBR=cpu@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235688

[alextaran1, 2013-11-15 09:51:06]

Please take a look

[Alexander Potapenko, 2013-11-15 10:32:18]

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
File third_party/instrumented_libraries/download_build_install.py (right):

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:15:
SUPPORTED_SANITIZERS = {"asan": "address"}
"Be consistent with your choice of string quote character within a file. Pick '
or " and stick with it."
(http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=String...)

Single quotes are more common in Chromium codebase.

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:19: "Changes
current working directory and restores it back automatically."
Doc strings should use """.

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:37: command =
'apt-get -s build-dep {0} | grep Inst | cut -d " " -f 2'.format(
It's more readable to split the string somewhere in the middle, e.g.:
  command = ('apt-get -s build-dep {0} | '
             'grep Inst | cut -d " " -f 2').format(library)

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:37: command =
'apt-get -s build-dep {0} | grep Inst | cut -d " " -f 2'.format(
Also consider using the % operator instead of the format() method.

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:39: command_result
= subprocess.Popen(command, stdout = subprocess.PIPE,
"Don't use spaces around the '=' sign when used to indicate a keyword argument
or a default parameter value."

http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Whites...

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:48:
os.putenv("CFLAGS", "-fsanitize={0} -g -fPIC -w".format(sanitizer_flag))
You don't need to change the actual environment of the current process.
Build a dict and pass it to subprocess.* functions as the |env| parameter.

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:69: with
ScopedChangeDirectory(library_directory), open(os.devnull, "w") as dev_null:
Please wrap this line.

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:77: with
ScopedChangeDirectory(subdirectories[0]):
You're assuming len(subdirectories) > 0. If this should always be true, update
the condition above to 'len(...) > 1'. Otherwise make sure to check it.

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:114: if
len(build_dependencies) != 0:
That's just 'if len(build_dependencies)'.
Overall, prefer implicit boolean conversions to explicit ones.

[alextaran1, 2013-11-15 12:00:51]

Please take another look

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
File third_party/instrumented_libraries/download_build_install.py (right):

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:15:
SUPPORTED_SANITIZERS = {"asan": "address"}
On 2013/11/15 10:32:19, Alexander Potapenko wrote:
> "Be consistent with your choice of string quote character within a file. Pick
'
> or " and stick with it."
>
(http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=String...)
> 
> Single quotes are more common in Chromium codebase.

Done.

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:19: "Changes
current working directory and restores it back automatically."
On 2013/11/15 10:32:19, Alexander Potapenko wrote:
> Doc strings should use """.

Done.

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:37: command =
'apt-get -s build-dep {0} | grep Inst | cut -d " " -f 2'.format(
On 2013/11/15 10:32:19, Alexander Potapenko wrote:
> Also consider using the % operator instead of the format() method.

After replacing "format" to "%" wrapping isn't needed anymore :)

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:39: command_result
= subprocess.Popen(command, stdout = subprocess.PIPE,
On 2013/11/15 10:32:19, Alexander Potapenko wrote:
> "Don't use spaces around the '=' sign when used to indicate a keyword argument
> or a default parameter value."
> 
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Whites...

Done.

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:48:
os.putenv("CFLAGS", "-fsanitize={0} -g -fPIC -w".format(sanitizer_flag))
On 2013/11/15 10:32:19, Alexander Potapenko wrote:
> You don't need to change the actual environment of the current process.
> Build a dict and pass it to subprocess.* functions as the |env| parameter.

Done.

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:69: with
ScopedChangeDirectory(library_directory), open(os.devnull, "w") as dev_null:
On 2013/11/15 10:32:19, Alexander Potapenko wrote:
> Please wrap this line.

Done.

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:77: with
ScopedChangeDirectory(subdirectories[0]):
On 2013/11/15 10:32:19, Alexander Potapenko wrote:
> You're assuming len(subdirectories) > 0. If this should always be true, update
> the condition above to 'len(...) > 1'. Otherwise make sure to check it.

Done.

https://codereview.chromium.org/60733027/diff/1/third_party/instrumented_libr...
third_party/instrumented_libraries/download_build_install.py:114: if
len(build_dependencies) != 0:
On 2013/11/15 10:32:19, Alexander Potapenko wrote:
> That's just 'if len(build_dependencies)'.
> Overall, prefer implicit boolean conversions to explicit ones.

Done.

[Alexander Potapenko, 2013-11-15 12:57:50]

LGTM with nits.

https://codereview.chromium.org/60733027/diff/60001/third_party/instrumented_...
File third_party/instrumented_libraries/download_build_install.py (right):

https://codereview.chromium.org/60733027/diff/60001/third_party/instrumented_...
third_party/instrumented_libraries/download_build_install.py:73: raise
Exception('Failed to download package %s' % parsed_arguments.library)
Please mind the 80-char limit.

https://codereview.chromium.org/60733027/diff/60001/third_party/instrumented_...
third_party/instrumented_libraries/download_build_install.py:76: if
len(subdirectories) > 1:
Please reinstate the '!= 1' condition per the offline discussion.

[Alexander Potapenko, 2013-11-15 13:04:00]

Nico, FYI

[commit-bot: I haz the power, 2013-11-15 13:04:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alextaran@chromium.org/60733027/60002

[commit-bot: I haz the power, 2013-11-15 13:18:58]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-15 13:28:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alextaran@chromium.org/60733027/60002

[commit-bot: I haz the power, 2013-11-15 14:05:15]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[Alexander Potapenko, 2013-11-15 14:09:27]

BTW s/rewrited/rewrote, s/from bash to/in.

On Fri, Nov 15, 2013 at 6:05 PM,  <commit-bot@chromium.org> wrote:
> Retried try job too often on win for step(s) compile
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
>
> https://codereview.chromium.org/60733027/



-- 
Alexander Potapenko
Software Engineer
Google Moscow

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexander Potapenko, 2013-11-15 14:09:56]

Dear CQ, this is still lgtm

[commit-bot: I haz the power, 2013-11-15 14:11:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alextaran@chromium.org/60733027/60002

[commit-bot: I haz the power, 2013-11-15 14:49:59]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[Nico, 2013-11-17 07:52:28]

Thanks!

https://codereview.chromium.org/60733027/diff/60002/third_party/instrumented_...
File third_party/instrumented_libraries/download_build_install.py (right):

https://codereview.chromium.org/60733027/diff/60002/third_party/instrumented_...
third_party/instrumented_libraries/download_build_install.py:6: # Downloads,
builds (with instrumentation) and installs shared libraries.
Program summaries of python scripts are usually in a docstring, not a comment
(i.e. write:

"""Downloads, builds (with instrumentation) and installs shared libraries."""

[Nico, 2013-11-17 07:52:37]

)

[commit-bot: I haz the power, 2013-11-18 07:23:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alextaran@chromium.org/60733027/470001

[commit-bot: I haz the power, 2013-11-18 07:53:04]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, chrome_frame_net_tests,
chrome_frame_tests, chrome_frame_unittests, chromedriver2_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, gpu_unittests, installer_util_unittests,
interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests,
mini_installer_test, net_unittests, ppapi_unittests, printing_unittests,
remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-18 07:56:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alextaran@chromium.org/60733027/760001

[commit-bot: I haz the power, 2013-11-18 07:56:40]

Can't process patch for file third_party/instrumented_libraries/fix_rpaths.sh.
Unsupported svn property operation.

[commit-bot: I haz the power, 2013-11-18 08:02:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alextaran@chromium.org/60733027/790001

[commit-bot: I haz the power, 2013-11-18 10:54:44]

Change committed as 235688

[Alexander Potapenko, 2013-11-18 11:02:33]

On 2013/11/18 10:54:44, I haz the power (commit-bot) wrote:
> Change committed as 235688

Please make sure to include the BUG=313751 into the related CLs next time.