Core mojo API for python.

Core mojo API for python.

Committed: https://crrev.com/28e16123231497d889ef08ebd917e80d5008ffbf
Cr-Commit-Position: refs/heads/master@{#291701}

[qsr, 2014-07-09 15:03:08]

This is not in a state ready for commit, but I'd like to have a first review so
that we can agree on the overall design.

 I will also need to work on having this build and the test run.

[pkl (ping after 24h if needed), 2014-07-10 18:03:23]

qsr: consider this the first pass. I didn't try to do any style-nits (except for
method naming) cause it is probably pre-mature.

https://codereview.chromium.org/377293002/diff/1/mojo/public/python/mojo/core...
File mojo/public/python/mojo/core.pyx (right):

https://codereview.chromium.org/377293002/diff/1/mojo/public/python/mojo/core...
mojo/public/python/mojo/core.pyx:266: def isValid(self):
Use function names like my_function() or MyFunction()
Recommend: def is_valid(self):
Same for other cases, e.g. write_message() instead of writeMessage().

https://codereview.chromium.org/377293002/diff/1/mojo/public/python/mojo/core...
mojo/public/python/mojo/core.pyx:347: - if code is any other value, data and
sizes will be None.
Since python is typeless, you can return a tuple of arbitrary type/length that
is conditional on the first one.

RESULT_OK: You return (code, buffer, handles)
RESULT_RESOURCE_EXHAUSTED: You return (code, buffer_size, handles_size)
Other: You return (code)

The caller of readMessage() must check code before interpreting result[1:]. This
may be less complex. It's a bit like varargs interpretation for printf.

https://codereview.chromium.org/377293002/diff/1/mojo/public/python/mojo/core...
mojo/public/python/mojo/core.pyx:428: view_buffer.wrap(out_buffer, out_size,
False)
It took me a little back-and-forth before I realize that the 3rd parameter means
read-only. How about being explicit and use read_only=False here?

[qsr, 2014-07-11 13:30:04]

https://codereview.chromium.org/377293002/diff/1/mojo/public/python/mojo/core...
File mojo/public/python/mojo/core.pyx (right):

https://codereview.chromium.org/377293002/diff/1/mojo/public/python/mojo/core...
mojo/public/python/mojo/core.pyx:266: def isValid(self):
On 2014/07/10 18:03:23, pklpkl wrote:
> Use function names like my_function() or MyFunction()
> Recommend: def is_valid(self):
> Same for other cases, e.g. write_message() instead of writeMessage().

 use lowercase with underscore. (In fact, didn't see the other one in the google
python code style:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?utm_source=Pyt...

https://codereview.chromium.org/377293002/diff/1/mojo/public/python/mojo/core...
mojo/public/python/mojo/core.pyx:347: - if code is any other value, data and
sizes will be None.
As discussed offline, I tend to prefer to always send the same number of
responses, so that you can do:

(res, data, sizes) = handle.readMessage(buffer)

https://codereview.chromium.org/377293002/diff/1/mojo/public/python/mojo/core...
mojo/public/python/mojo/core.pyx:428: view_buffer.wrap(out_buffer, out_size,
False)
On 2014/07/10 18:03:23, pklpkl wrote:
> It took me a little back-and-forth before I realize that the 3rd parameter
means
> read-only. How about being explicit and use read_only=False here?

Done.

[qsr, 2014-08-20 08:49:51]

gentle ping?

[pkl (ping after 24h if needed), 2014-08-20 11:05:07]

ack

[pkl (ping after 24h if needed), 2014-08-21 09:32:48]

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
File mojo/public/python/mojo/system.pyx (right):

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:87: raise TypeError("Unable to read buffer.")
style: Be consistent with string quotes. Stick with either single or double
quotes.

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:99: """Slice the given buffer, reducing it to
the given size. Return None if None
nit: Line break with a blank line after "...given size." and then put "Return
None if..." .

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:102: if not buffer:
How about test with this?
  if type(buffer) not in (types.ListType, types.TupleType):
    return buffer

This safely filters out bad input like buffer=True, buffer=1, buffer="x", etc.

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:189: handles_and_signals: list of pairs of
handle and signal.
nit: s/pairs/tuples/

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:215: The buffer field contain the python
buffer where data can be read or write.
s/contain/contains/
s/write/written/

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:245: self.end(0)
Looks like you are doing some clean up work here by calling end(0). Wouldn't it
be an assert/error if self._buffer is not None because end() was not called?

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:250: The buffer field contain the python
buffer where data can be read or write.
ditto: contains. written.

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:251: When done with the buffer, the |unmao|
method must be called.
s/unmao/unmap/

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:330: Writes a message to the message pipe.
style: move this to the line with """

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:504: def begin_read_data(self, min_size =
None, flags=READ_DATA_FLAG_NONE):
style: no space around =

https://chromiumcodereview.appspot.com/377293002/diff/320001/third_party/cyth...
File third_party/cython/cp_python_binary_modules.py (right):

https://chromiumcodereview.appspot.com/377293002/diff/320001/third_party/cyth...
third_party/cython/cp_python_binary_modules.py:4: 
Please add docstring. This looks like something that you will invoke from
command line, so it would be helpful to have a docstring describing what this is
for and what the arguments mean (instead of requiring everyone to read the
code).

https://chromiumcodereview.appspot.com/377293002/diff/320001/third_party/cyth...
File third_party/cython/python_flags.py (right):

https://chromiumcodereview.appspot.com/377293002/diff/320001/third_party/cyth...
third_party/cython/python_flags.py:4: 
Please add docstring. This looks like something that you will invoke from
command line, so it would be helpful to have a docstring describing what this is
for and what the arguments mean (instead of requiring everyone to read the
code).

https://chromiumcodereview.appspot.com/377293002/diff/320001/third_party/cyth...
third_party/cython/python_flags.py:45: result += [ '%s/lib' %
sysconfig.get_config_vars('prefix')[0] ]
Can you use result.append(...) so it is consistent with the earlier uses?

[qsr, 2014-08-21 12:10:05]

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
File mojo/public/python/mojo/system.pyx (right):

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:87: raise TypeError("Unable to read buffer.")
On 2014/08/21 09:32:47, pklpkl wrote:
> style: Be consistent with string quotes. Stick with either single or double
> quotes.

Done.

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:99: """Slice the given buffer, reducing it to
the given size. Return None if None
On 2014/08/21 09:32:47, pklpkl wrote:
> nit: Line break with a blank line after "...given size." and then put "Return
> None if..." .

Done.

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:102: if not buffer:
On 2014/08/21 09:32:47, pklpkl wrote:
> How about test with this?
>   if type(buffer) not in (types.ListType, types.TupleType):
>     return buffer
> 
> This safely filters out bad input like buffer=True, buffer=1, buffer="x", etc.

 This is an internal method. Do we want to pay the runtime cost for internal
validation?

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:189: handles_and_signals: list of pairs of
handle and signal.
On 2014/08/21 09:32:47, pklpkl wrote:
> nit: s/pairs/tuples/

Done.

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:215: The buffer field contain the python
buffer where data can be read or write.
On 2014/08/21 09:32:47, pklpkl wrote:
> s/contain/contains/
> s/write/written/

Done.

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:245: self.end(0)
On 2014/08/21 09:32:47, pklpkl wrote:
> Looks like you are doing some clean up work here by calling end(0). Wouldn't
it
> be an assert/error if self._buffer is not None because end() was not called?

Hum... I was thinking about letting this pass, but that is probably an error
from the user, changed.

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:250: The buffer field contain the python
buffer where data can be read or write.
On 2014/08/21 09:32:47, pklpkl wrote:
> ditto: contains. written.

Done.

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:251: When done with the buffer, the |unmao|
method must be called.
On 2014/08/21 09:32:48, pklpkl wrote:
> s/unmao/unmap/

Done.

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:330: Writes a message to the message pipe.
On 2014/08/21 09:32:47, pklpkl wrote:
> style: move this to the line with """

Done.

https://chromiumcodereview.appspot.com/377293002/diff/320001/mojo/public/pyth...
mojo/public/python/mojo/system.pyx:504: def begin_read_data(self, min_size =
None, flags=READ_DATA_FLAG_NONE):
On 2014/08/21 09:32:47, pklpkl wrote:
> style: no space around =

Done.

https://chromiumcodereview.appspot.com/377293002/diff/320001/third_party/cyth...
File third_party/cython/cp_python_binary_modules.py (right):

https://chromiumcodereview.appspot.com/377293002/diff/320001/third_party/cyth...
third_party/cython/cp_python_binary_modules.py:4: 
On 2014/08/21 09:32:48, pklpkl wrote:
> Please add docstring. This looks like something that you will invoke from
> command line, so it would be helpful to have a docstring describing what this
is
> for and what the arguments mean (instead of requiring everyone to read the
> code).

Done.

https://chromiumcodereview.appspot.com/377293002/diff/320001/third_party/cyth...
File third_party/cython/python_flags.py (right):

https://chromiumcodereview.appspot.com/377293002/diff/320001/third_party/cyth...
third_party/cython/python_flags.py:4: 
On 2014/08/21 09:32:48, pklpkl wrote:
> Please add docstring. This looks like something that you will invoke from
> command line, so it would be helpful to have a docstring describing what this
is
> for and what the arguments mean (instead of requiring everyone to read the
> code).

Done.

https://chromiumcodereview.appspot.com/377293002/diff/320001/third_party/cyth...
third_party/cython/python_flags.py:45: result += [ '%s/lib' %
sysconfig.get_config_vars('prefix')[0] ]
On 2014/08/21 09:32:48, pklpkl wrote:
> Can you use result.append(...) so it is consistent with the earlier uses?

Done.

[pkl (ping after 24h if needed), 2014-08-21 13:01:41]

lgtm

[qsr, 2014-08-21 13:21:49]

Darin, Trung, Chris, do you want to take a look before I push this?

[Chris Masone, 2014-08-21 13:33:54]

On 2014/08/21 13:21:49, qsr wrote:
> Darin, Trung, Chris, do you want to take a look before I push this?

I believe we're trying to keep the GN build in sync with the GYP build as we go
along with Mojo, since the latter will be deprecated in the next several months.
Can you add that, please?

[qsr, 2014-08-22 13:02:06]

On 2014/08/21 13:33:54, Chris Masone wrote:
> On 2014/08/21 13:21:49, qsr wrote:
> > Darin, Trung, Chris, do you want to take a look before I push this?
> 
> I believe we're trying to keep the GN build in sync with the GYP build as we
go
> along with Mojo, since the latter will be deprecated in the next several
months.
> Can you add that, please?

 Done.

[Chris Masone, 2014-08-24 19:59:41]

On 2014/08/22 13:02:06, qsr wrote:
> On 2014/08/21 13:33:54, Chris Masone wrote:
> > On 2014/08/21 13:21:49, qsr wrote:
> > > Darin, Trung, Chris, do you want to take a look before I push this?
> > 
> > I believe we're trying to keep the GN build in sync with the GYP build as we
> go
> > along with Mojo, since the latter will be deprecated in the next several
> months.
> > Can you add that, please?
> 
>  Done.

I'm OK with landing this, but would like to revisit the choice to check cython
in directly offline.

[qsr, 2014-08-25 08:45:33]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-08-25 08:46:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qsr@chromium.org/377293002/400001

[commit-bot: I haz the power, 2014-08-25 08:46:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-25 08:46:32]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[blundell, 2014-08-25 08:48:51]

RSLGTM

[qsr, 2014-08-25 08:50:28]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-08-25 08:51:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qsr@chromium.org/377293002/400001

[commit-bot: I haz the power, 2014-08-25 11:18:02]

Committed patchset #21 (400001) as d4127069d77fb9c569cce2acc3a8a5b48c986ff8

[qsr, 2014-08-25 16:01:31]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-08-25 16:02:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qsr@chromium.org/377293002/420001

[commit-bot: I haz the power, 2014-08-25 17:01:14]

Committed patchset #22 (420001) as 83b9b15f0e3f12f86091c3bf99ef57c81f25cec3

[commit-bot: I haz the power, 2014-09-10 02:33:48]

Patchset 22 (id:??) landed as
https://crrev.com/94c1ddb7dfdafaec4a5045cdf2067a7ade6ea6ad
Cr-Commit-Position: refs/heads/master@{#291662}

[commit-bot: I haz the power, 2014-09-10 02:35:12]

Patchset 22 (id:??) landed as
https://crrev.com/28e16123231497d889ef08ebd917e80d5008ffbf
Cr-Commit-Position: refs/heads/master@{#291701}