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.

Sign up for GitHub

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

Fix build without cython (#3162) #3164

Closed
wants to merge 2 commits into from
Closed

Fix build without cython (#3162) #3164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d993f55
Fix build without cython (#3162)
apatrushev Jul 29, 2018
493a435
added myself to contributors as requested in PR procedure
apatrushev Jul 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES/3162.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Build without cython fixed
1 change: 1 addition & 0 deletions CONTRIBUTORS.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ Andrew Svetlov
Andrii Soldatenko
Antoine Pietri
Anton Kasyanov
Anton Patrushev
Anton Zhdan-Pushkin
Arthur Darcet
Ben Bader
Expand Down
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def run(self):
def build_extension(self, ext):
try:
build_ext.build_extension(self, ext)
except (DistutilsExecError,
except (CCompilerError, DistutilsExecError,
Copy link
Member

Choose a reason for hiding this comment

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

This is a workaround. We need a real fix, finding out why the file is missing or having a clear explanation of why this is happening.

Copy link
Author

@apatrushev apatrushev Jul 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Explanation is easy - build process relied on assumption that if we have no cython, than we will have compiler error and ignore extensions bundling. Later runtime will work with pure python implementations. This file is missing because cython is not available during build. So I can not understand what do you mean by real fix?

Copy link
Author

Choose a reason for hiding this comment

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

One more note: I just reintroduced the way how it worked before and was broken in 28f1519, as I mentioned in the issue.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, it might make sense. Let's wait for @asvetlov.
P.S. I still want to see this tested in CI.

Copy link
Author

Choose a reason for hiding this comment

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

Nope. We already discussed the problem with @asvetlov in #3162 and I probably caught his idea and pain about development environment. Though the build is broken anyway: it is impossible to build/install from sources without gcc. So I plan to improve setup.py.

Copy link
Member

Choose a reason for hiding this comment

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

And while you're on it you might want to extend docs to mention how things work.

Copy link
Author

@apatrushev apatrushev Jul 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'll require adding env combos to Travis CI as mandatory anyway.

@asvetlov mentioned that this is the bad idea, because it will increase even more the testing time.

Copy link
Member

Choose a reason for hiding this comment

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

Not really, just add it and then I'll make sure that it doesn't hit performance. I'll basically do to it what I did to OS X and it'll be a fine compromise: running those only on cron as opposed to having them in each build. Andrew trusts me to handle CI.

P.S. If we recognize that we need it on each commit we could always move it to another CI service.

Copy link
Member

Choose a reason for hiding this comment

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

Remark: while working on changing setup.py it might be worth adding some env var for toggling strict mode for dev.

It could be smth like:

import os
...
class ve_build_ext(build_ext):
    ...
    def build_extension(self, ext):
        valid_build_exceptions = {CCompilerError, DistutilsExecError, DistutilsPlatformError, ValueError}
        suppressed_build_exceptions = set()

        if os.getenv('AIOHTTP_DEV_MODE'):
            suppressed_build_exceptions = {CCompilerError, DistutilsExecError, DistutilsPlatformError}

        try:
            build_ext.build_extension(self, ext)
        except tuple(valid_build_exceptions - suppressed_build_exceptions) as build_exc:
            raise BuildFailed() from build_exc

cc: @asvetlov

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that's what I planned to implement.

DistutilsPlatformError, ValueError):
raise BuildFailed()
Copy link
Member

Choose a reason for hiding this comment

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

It's also useful to add from original_exception part.


Expand Down