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

Found cause for issue #18. #26

Merged
merged 1 commit into from
May 18, 2024
Merged

Found cause for issue #18. #26

merged 1 commit into from
May 18, 2024

Conversation

lschoe
Copy link
Collaborator

@lschoe lschoe commented May 18, 2024

Is calling uv.uv_disable_stdio_inheritance() needed at all?

Is calling uv.uv_disable_stdio_inheritance() needed at all?
@Vizonex
Copy link
Owner

Vizonex commented May 18, 2024

@lschoe Depends. I think it should be feature if I should be honest with you. I implemented this in the hopes that there would be a performance increase.

@Vizonex Vizonex merged commit 380fbf2 into main May 18, 2024
@lschoe
Copy link
Collaborator Author

lschoe commented May 19, 2024

OK @Vizonex, let's make a (mental) note about this, also have it in the commit history, but go ahead and remove it for the time being. These potential performance improvements can be investigated later, once winloop is integrated with uvloop, I would say.

@Vizonex
Copy link
Owner

Vizonex commented May 19, 2024

@lschoe I guess the only problem now might be Cython's recent deprecation of macros. Other than that I am overall very excited about these future changes.

@lschoe
Copy link
Collaborator Author

lschoe commented May 20, 2024

Hi @Vizonex . Oh, nice! Yes, indeed the use of Cython DEF should ultimately be dropped from the code. But for now I would keep it in where uvloop also still has it, and when uvloop drops it, winloop can follow suit.

The present differences between winloop and uvloop are converging steadily to the essential ones. By using things like if system.PLATFORM_IS_WINDOWS we can insert some Windows specific code at runtime. And in the C header files we can still use DEF. And similar things, as advocated in Deprecation of DEF / IF.

@lschoe
Copy link
Collaborator Author

lschoe commented May 21, 2024

Hi @Vizonex, I did another round of updates. I've added some unit tests from uvloop (the ones that do not fail). Also, now using uvlib as a git submodule. Many changes throughout, also making setup.py more like uvloop. Only left the macros that seem to be essential ("WIN32_LEAN_AND_MEAN", 1) and ("_WIN32_WINNT", "0x0602").

Would be nice if you can update the yaml files to also build a Python 3.8 version? That runs as well as 3.9, so until uvloop drops 3.8 we can let winloop support 3.8.

@Vizonex
Copy link
Owner

Vizonex commented May 23, 2024

@lschoe I would make one but I do not understand how to compile workflows I've tried even just compiling a nightly build. It's just too difficult for me. However I am watching these updates closely I may even try compiling what has been done as a testrun.

@lschoe
Copy link
Collaborator Author

lschoe commented May 23, 2024

Hi @Vizonex, I saw your update to the README.md. I would be a bit more cautious with this, as I think the merge with uvloop will take quite a bit more time. They will only consider this if everything is kind of flawless. For that most of the uvloop tests should pass, on all platforms, which will still be a lot of work to accomplish. Also related to issues like #22, which involves use of pipes (I ran the program of #22 on Unix, there it works with uvloop, but on Windows it does not with winloop, but also not with the asyncio loop).

Given that, I would recommend that you release version 0.1.4 now, or soon, such that people can try it and test it. Also important to include the Python 3.8 version this time around. Then Python 3.8-12 are covered, just as uvloop does. The fix for issue #18 will also be included. And, not unimportantly, the requirement for cython should be gone (hence people need to wonder or worry about needing cython for just running winloop).

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.

2 participants