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

Make autogen work under Windows+MSVC #440

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

KubaO
Copy link
Contributor

@KubaO KubaO commented Jun 20, 2023

  1. Default encoding on Windows isn't UTF-8. This caused autogen to mangle the documentation files.
  2. Add detection of MSVC so that autogen will work under msvc command line.

This helps when using hpy under MSVC.

Copy link
Contributor

@fangerer fangerer left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. In general, looks good to me. Maybe just remove the unused variable and then I'm happy to merge (if tests are passing, ofc).

hpy/tools/autogen/parse.py Show resolved Hide resolved
hpy/tools/autogen/parse.py Show resolved Hide resolved
@KubaO
Copy link
Contributor Author

KubaO commented Jun 27, 2023

We don't care about Python 2.7 anymore, right?

Autogen has to be invoked from the MSVC command line so that cl.exe is in path.
@fangerer
Copy link
Contributor

We don't care about Python 2.7 anymore, right?

Hi @KubaO, please excuse my very late answer. We don't really care about Python 2.7 in the sense that we don't run or support HPy on Python 2.7. However, we are running tests on PyPy and some of the test files therefore also need to be Python 2.7 compatible. Anyway, @mattip already has a fix (see #444 ).

@mattip
Copy link
Contributor

mattip commented Jul 24, 2023

Just to be clear: #444 fixes the CI startup failure for 2.7, but if you want to use open with encoding, that won't work on 2.7. So if these code paths are hit when running

python2.7 test/check_py27_compat.py

then the 2.7 CI job will still fail, since that is the command it runs.

Running this PR locally, everything here seems fine.

@mattip
Copy link
Contributor

mattip commented Mar 7, 2024

Close/reopen to trigger CI

@mattip mattip closed this Mar 7, 2024
@mattip mattip reopened this Mar 7, 2024
@fangerer fangerer merged commit 176724c into hpyproject:master Mar 7, 2024
38 checks passed
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.

3 participants