Skip to content

Do not fail when gen_protos cannot be imported#3279

Closed
aaltay wants to merge 1 commit intoapache:masterfrom
aaltay:lnt
Closed

Do not fail when gen_protos cannot be imported#3279
aaltay wants to merge 1 commit intoapache:masterfrom
aaltay:lnt

Conversation

@aaltay
Copy link
Copy Markdown
Member

@aaltay aaltay commented Jun 1, 2017

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.04%) to 70.698% when pulling 930a1e3 on aaltay:lnt into 63bcd39 on apache:master.

@robertwb
Copy link
Copy Markdown
Contributor

robertwb commented Jun 1, 2017

When would we expect this to happen?

@asfbot
Copy link
Copy Markdown

asfbot commented Jun 1, 2017

Build finished.
--none--

@aaltay
Copy link
Copy Markdown
Member Author

aaltay commented Jun 1, 2017

It happens, if the sdk is packaged without the egg_info. In that case pip install, triggers python setup.py egg_info. Even though this cmd does not requires protos, it fails because packaged SDK does not contain the gen_protos.py.

@robertwb
Copy link
Copy Markdown
Contributor

robertwb commented Jun 2, 2017

How about in that case wrapping the commands with classes that raise an error? This way we never silently fail to generate the protos when we need them.

@robertwb
Copy link
Copy Markdown
Contributor

robertwb commented Jun 2, 2017

Alternatively, move the import into the run method.

@aaltay
Copy link
Copy Markdown
Member Author

aaltay commented Jun 2, 2017

Would not it be the same problem, if we move the import into the run method? Then it will fail when run is called.

@robertwb
Copy link
Copy Markdown
Contributor

robertwb commented Jun 2, 2017 via email

@aaltay
Copy link
Copy Markdown
Member Author

aaltay commented Jun 6, 2017

@robertwb Maybe I am missing something, but moving the import into the run method fails the same way. Because egg_info, triggers running of build_py. (That was the reason for original failure, egg_info did indeed caused a call to generate_protos_first and that returns a wrapped cmd.)

@robertwb
Copy link
Copy Markdown
Contributor

robertwb commented Jun 6, 2017

Ah, I wasn't aware that egg_info called build_py.

LGTM.

@aaltay
Copy link
Copy Markdown
Member Author

aaltay commented Jun 6, 2017

Thank you, merging.

@asfgit asfgit closed this in 7c608c3 Jun 6, 2017
@aaltay aaltay deleted the lnt branch August 28, 2017 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants