Python style guide

[Bruce Dawson]

I was looking at the Chromium Python style guide and I came across this sentence:

>  you should not use vpython during PRESUBMIT checks

This surprised me because functions like input_api.canned_checks.RunUnitTests use vpython and vpython3. Is the guidance wrong, or does RunUnitTests need to be updated?

Also, while working on presubmits I have found several scripts that fail on Windows. Two of the avoidable problems are related to different defaults for how text files are read and written. Specifically, Python defaults to \r\n line endings when writing files, and Python 3 defaults to the cp1252 encoding when reading and writing files.

In order to avoid latent compatibility problems I would like to propose that the Python style guide be updated to recommend specifying encoding='utf-8' whenever a text file is opened for reading or writing, and newline='' (or newline='\n') whenever opening a text file for writing.

Thoughts?

--

Bruce Dawson, he/him

[Mike Frysinger]

makes sense to me.  i implemented an encoding='utf-8' lint for `cros lint` but haven't yet deployed it because we need to clean up our codebase a bit more first :/.

https://issuetracker.google.com/issues/187789221

-mike

--
You received this message because you are subscribed to the Google Groups "python" group.
To unsubscribe from this group and stop receiving emails from it, send an email to python+un...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/python/CAE5mQiMbymHRAJkB3q4KQyrLFNnUni9Sik%2BVPUP2N4K5C%3D1f%3Dg%40mail.gmail.com.

[Andrew Grieve]

I know vpython isn't ready to be used by commands invoked by ninja due to added start-up overhead, but I don't know why it wouldn't be fine for presubmit and tests.

encoding='utf8' sgtm.

newline is a bit subtle, so probably also worth a mention. Disabling translation sgtm.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/python/CAAbOSckfjRNpbOqmwJyRjK_udsfP%3DjkGC%2BqyG24DJBEdBpOxMA%40mail.gmail.com.

[Mike Frysinger]

[ +Brian ]

iirc, Brian said they made some perf improvements to vpython startup.  i don't know how many times python is invoked from ninja rules (doesn't seem like it'd be that many unless it's a wrapper around the compiler driver).  might be worth getting updated stats.

-mike

[Brian Ryner]

+Chenlin Fan

Yes, we've definitely made some improvements and there are more coming. That said, there is still overhead, and it's more noticeable on Windows (due to creating a subprocess, vs. execve on Linux/Mac) -- but it's going to be on the order of a couple hundred milliseconds. I don't know what the specific performance targets for your presubmits are, but it seems reasonable to me to use it where it's necessary.

Updated measurements on all platforms would be good, of course. Note that the overhead on developer workstations tends to be more than on Swarming, because some of it comes from depot_tools' vpython wrapper.

--

-Brian

[Peter Wen]

Yup, you're exactly right, python is invoked as a wrapper around almost all our android build tools (e.g. turbine.py, compile_java.py, dex.py, etc under //build/android/gyp). So startup costs are paid multiple times per java target (since each has multiple sub-targets using python wrappers), and we have ~2000 java targets. So we do pay the python startup costs a lot in each android build. :)

Peter

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/python/CAAbOSckB9HF53YG_C8rCcjv_JAhGtJMt-fZ3S3wArnRCczZtGQ%40mail.gmail.com.

[Bruce Dawson]

I did a test run last night of "git cl presubmit --all --force" with vpython/vpython3 instrumented so that I could see how often it is invoked. The answer is 554 times over a ~90 minute run. So, in that context the cost is minor.

pretty_print.py and json_schema_compiler\idl_schema.py seemed to be the most commonly invoked vpython invocations so it is possible that there are some CLs where one of these scripts is invoked enough times for the startup cost to be a factor, but I doubt it is significant.

I'm working on some possible updates to the Python style guide and I am deleting the clause that says that vpython shouldn't be used in presubmits.

--

Bruce Dawson, he/him

[Bruce Dawson]

My proposed tweaks to the Python style guide can be found here: crrev.com/c/3823598

--

Bruce Dawson, he/him