Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve non-functional Cython error messages #402

Merged
merged 3 commits into from Feb 20, 2019

Conversation

Projects
None yet
3 participants
@alubbock
Copy link
Member

commented Jan 9, 2019

On Windows, a lack of a functional compiler for Cython is often met with
the message "Unable to find vcvarsall.bat", which is cryptic for newbies.
This update adds a more descriptive warning complete with URL, logging,
and a consolidated "catch" condition.

Improve non-functional Cython error messages
On Windows, a lack of a functional compiler for Cython is often met with
the message "Unable to find vcvarsall.bat", which is cryptic for newbies.
This update adds a more descriptive warning complete with URL, logging,
and a consolidated "catch" condition.
@coveralls

This comment has been minimized.

Copy link

commented Jan 9, 2019

Coverage Status

Coverage decreased (-1.2%) to 78.398% when pulling 23d0104 on alubbock:cython_detection into b8e036c on pysb:master.

@coveralls

This comment has been minimized.

Copy link

commented Jan 9, 2019

Coverage Status

Coverage decreased (-0.03%) to 78.882% when pulling 991fcd3 on alubbock:cython_detection into d3af6f1 on pysb:master.

distutils.errors.CompileError,
ImportError,
ValueError) as e:
if isinstance(e, ValueError) and (

This comment has been minimized.

Copy link
@jmuhlich

jmuhlich Feb 5, 2019

Member

This logic was already a little obscure and it's duplicated here and below. The immediate debug message previously made it fairly clear what was being checked, but with this change it's less apparent what's going on. Maybe this is a good time to refactor it. Could you refactor it into a well-named predicate function on e with an explanation of what it's looking for? Also I don't think rewriting the arg-checking term as a disjunction of negations made it any better... I do prefer the old form. Or maybe just e.args == ('Symbol table not found',) which actually covers the whole thing at once?

distutils.errors.DistutilsPlatformError,
ValueError) as e:
if isinstance(e, distutils.errors.DistutilsPlatformError) and \
str(e) != 'Unable to find vcvarsall.bat':

This comment has been minimized.

Copy link
@jmuhlich

jmuhlich Feb 5, 2019

Member

Does this error not also occur under weave.inline? Can we unify all the C compiler breakage handling into one place?

@alubbock

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

I've refactored the logic into a separate function. Hopefully that's a little clearer now?

@alubbock alubbock merged commit 75355a5 into pysb:master Feb 20, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alubbock alubbock deleted the alubbock:cython_detection branch Feb 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.