Skip to content

Migrate the last of the Python code from Google-internal source control to git.#274

Merged
ctiller merged 2 commits intogrpc:masterfrom
nathanielmanistaatgoogle:python-introduction
Feb 2, 2015
Merged

Migrate the last of the Python code from Google-internal source control to git.#274
ctiller merged 2 commits intogrpc:masterfrom
nathanielmanistaatgoogle:python-introduction

Conversation

@nathanielmanistaatgoogle
Copy link
Copy Markdown
Contributor

Please review @nicolasnoble @murgatroid99 @tbetbetbe @ctiller. Changes made include:
(1) Path migrations.
(2) Migration from grpc_call_start_invoke to grpc_call_invoke.
(3) Port auto-selection in tests.

@nathanielmanistaatgoogle
Copy link
Copy Markdown
Contributor Author

Also please take a look @soltanmm.

@ctiller
Copy link
Copy Markdown
Member

ctiller commented Jan 29, 2015

start_invoke transition LGTM

@ctiller
Copy link
Copy Markdown
Member

ctiller commented Jan 29, 2015

Where should I look for the port selection logic?

@nicolasnoble
Copy link
Copy Markdown
Contributor

I don't really have any concern with this PR.

@nathanielmanistaatgoogle
Copy link
Copy Markdown
Contributor Author

@ctiller port selection logic is exposed via the (Python) server's add_http2_addr method and the Fore's start method.

@craigcitro Please also take a look and merge if satisfied?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'[::]:0' (and in the other places)

... make it ipv4&6 compatible

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done; thanks for pointing these out.

@nathanielmanistaatgoogle
Copy link
Copy Markdown
Contributor Author

(All outstanding comments addressed.)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, shouldn't it stay in the project root dir for installation purposes? (this is the usual way setup.py works in my experience ...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's tough in a multi-language repository. For the scope of this commit: no, it needn't. Outside of this commit: maybe? I have a feeling that it won't move back to the repository root - see the way that for testing the Python code is currently installed into the virtual environment with "python2.7_virtual_environment/bin/pip install src/python".

I've removed the out-of-date TODO.

@craigcitro
Copy link
Copy Markdown

one comment on a TODO, otherwise LGTM. (i can't merge the PR, since i don't have write access to the repo.)

The source code is moved from src/python to
src/python/src. A setup.py is added at
src/python. The build_python.sh and
run_python.sh scripts are updated to build
and run the Python tests by building a
package and installing it in the developer's
Python 2.7 virtual environment.
@nathanielmanistaatgoogle
Copy link
Copy Markdown
Contributor Author

Ping to those reviewers with permissions to merge this - I believe all outstanding comments have been addressed.

Also I used the process outlined at http://stackoverflow.com/questions/1186535/how-to-modify-a-specified-commit to remove the TODO in setup.py as it never actually applies at any stage of the history of this branch.

ctiller added a commit that referenced this pull request Feb 2, 2015
…tion

Migrate the last of the Python code from Google-internal source control to git.
@ctiller ctiller merged commit 97798ae into grpc:master Feb 2, 2015
@ctiller ctiller deleted the python-introduction branch February 2, 2015 15:36
@lock lock bot locked as resolved and limited conversation to collaborators Feb 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants