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

start to support windows #125

Merged
merged 51 commits into from
May 1, 2021
Merged

start to support windows #125

merged 51 commits into from
May 1, 2021

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Nov 29, 2020

it passes one test, but fails to link on others. Solved.

Support windows in build and tests

hpy/universal/src/ctx_meth.c Outdated Show resolved Hide resolved
@@ -12,7 +12,10 @@
class TestParseItem(HPyTest):
def make_parse_item(self, fmt, type, hpy_converter):
mod = self.make_module("""
__attribute__((unused)) static inline
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we should hide this difference behind a macro and put it in hpy.h. It looks like a generally useful feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but hpy.h is not imported where the macro is needed

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, it's included automatically by make_module:

hpy/test/support.py

Lines 56 to 61 in d5c8442

def expand(self):
self.defines_table = []
self.type_table = []
self.output = ['#include <hpy.h>']
for line in self.src.split('\n'):
match = self.r_marker.match(line)

@mattip
Copy link
Contributor Author

mattip commented Feb 22, 2021

I wonder what construct MSVC will support instead of HPy items[] = {};, which it errors out on.

// args is null, but kw is not, so we need to create an empty args tuple
// for CPython's PyObject_Call
HPy items[] = {};
HPy empty_tuple = HPyTuple_FromArray(ctx, items, 0);

@mattip
Copy link
Contributor Author

mattip commented Feb 23, 2021

Happy for help if someone sees how to fix the int/long/long long failures. The current proof-of-concept failure is due to trying to set up a virtualenv where one already exists.

@mattip
Copy link
Contributor Author

mattip commented Feb 25, 2021

It turns out the proof-of-concept run was confusing various virtual environments since it used VENV=... without declaring that a local variable.

This is evident when running the wheel() function: it first sets VENV, then calls _build_wheel. This sets VENV (overriding the value previously set), creates a virtualenv, and installs hpy and builds the poc wheel. Then _build_wheel exits. Now the wheel function creates a virtualenv, activates it, installs the poc wheel, and run tests. Note it did not install hpy into the virtualenv. Before this PR, that worked because the value of VENV set in _build_wheel changed the value set before the call to _build_wheel. Now that the VENV is local, running tests failed because hpy was not installed into the last virtualenv.

In 26ef55b I added a call to _install_hpy to fix this.

@stonebig
Copy link

stonebig commented Apr 21, 2021

Can it be part of upcoming PyPy-7.3.5 64bit for Windows ? It could help for your May 11th PyCon Langage Summit presentation

@hodgestar
Copy link
Contributor

Can it be part of upcoming PyPy-7.3.5 64bit for Windows ? It could help for your May 11th PyCon Langage Summit presentation

PyPy implements its own hpy.universal support so Windows support for HPy on PyPy is separate to the Windows support for HPy on CPython in this PR. I don't know offhand what the plans are for PyPy 7.3.5.

Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

These are the issues that I think are still outstanding:

__attribute__((unused)) static void hpy_magic_dump(HPy h)
#ifndef _MSC_VER
__attribute__((unused))
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

We should define HPy_UNUSED in macros.h and then use that everywhere.

// signature as HPy_MODINIT

// Copied from Python's exports.h
#ifndef Py_EXPORTED_SYMBOL
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call this HPy_EXPORTED_SYMBOL and add it to macros.h?

@@ -2,6 +2,11 @@
#define HPY_UNIVERSAL_HPYTYPE_H

#include <stdbool.h>
#ifdef __GNUC__
#define HPyAPI_UNUSED __attribute__((unused)) static inline
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this should also be moved to macros.h.

@hodgestar
Copy link
Contributor

@mattip @antocuni I'm also happy to merge this PR and address the minor issues in a follow up PR.

@mattip
Copy link
Contributor Author

mattip commented May 1, 2021

Merging now and fixing up the other issues in subsequent PRs sounds good to me. This PR is already quite large.

@hodgestar hodgestar merged commit ee0e6b1 into hpyproject:master May 1, 2021
@hodgestar
Copy link
Contributor

Merged!

@stonebig
Copy link

stonebig commented Oct 1, 2023

There is no Windows wheel on pypi for hpy-0.9.0
Is it to happen only for 1.0 ?

@mattip
Copy link
Contributor Author

mattip commented Oct 1, 2023

The windows wheels are explicity excluded. Will this github workflow run without a tag, on a PR to debug the windows builds?

@fangerer
Copy link
Contributor

Sorry for my late answer. When we've migrated to GitHub Workflows, building wheels for Windows using cibuildwheel did not work. Since then, we actually never tested. However, since we run tests on Windows as well, it should in theory work. If that's still something people are interested in, I can give it a try.

@stonebig
Copy link

Sorry for my late answer. When we've migrated to GitHub Workflows, building wheels for Windows using cibuildwheel did not work. Since then, we actually never tested. However, since we run tests on Windows as well, it should in theory work. If that's still something people are interested in, I can give it a try.

if hpy is to become a PEP, it would make sense to at least check it can generate a Windows wheel

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.

7 participants