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

Replace uses of python distutils library #2888

Closed
wants to merge 1 commit into from
Closed

Conversation

brjsp
Copy link

@brjsp brjsp commented Jul 25, 2023

It is not available anymore in Python 3.12: https://peps.python.org/pep-0632/

@cclauss
Copy link
Contributor

cclauss commented Aug 25, 2023

Please rebase this pull request so we can see if the tests pass.

@brjsp
Copy link
Author

brjsp commented Sep 27, 2023

@cclauss done

@Trott
Copy link
Member

Trott commented Sep 28, 2023

The CI test failures here will be fixed by #2908.

@cclauss
Copy link
Contributor

cclauss commented Sep 28, 2023

#2908 is now merged so please rebase this PR so we can be sure that the tests pass.

@Trott
Copy link
Member

Trott commented Sep 28, 2023

#2908 is now merged so please rebase this PR so we can be sure that the tests pass.

The rebase fixed the test failures on ubuntu and macos, but there are failures on Windows that are still happening. Windows didn't fail on the main branch, so I guess (but don't know) that those failures are related to the change in this PR.

@cclauss
Copy link
Contributor

cclauss commented Oct 4, 2023

Perhaps we need to pip install setuptools or packaging or something else on Windows.

@zooba
Copy link

zooba commented Oct 4, 2023

Perhaps we need to pip install setuptools or packaging or something else on Windows.

Yes, you'll want to include packaging for this change.

@cclauss
Copy link
Contributor

cclauss commented Oct 4, 2023

https://github.com/nodejs/node-gyp/pull/2862/files The installation of packaging is insufficient.

@zooba
Copy link

zooba commented Oct 4, 2023

But not because of packaging, it seems to be because of this:

Starting test 'build simple addon in path with non-ascii characters'
Traceback (most recent call last):
  File "D:\a\node-gyp\node-gyp\test\fixtures\test-charmap.py", line 31, in <module>
    print(main())
  File "D:\a\node-gyp\node-gyp\test\fixtures\test-charmap.py", line 26, in main
    print(textmap[encoding])
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\encodings\cp1252.py", line 19, in encode
Test 'build simple addon in path with non-ascii characters' skipped in 98 ms
    return codecs.charmap_encode(input,self.errors,encoding_table)[0]

UnicodeEncodeError: 'charmap' codec can't encode character '\u012b' in position 3: character maps to <undefined>

If you want Python to use utf-8 instead of the platform default encoding, set PYTHONIOENCODING=utf-8 environment variable before launching it. If you're launching it from inside your code, you'll want to set it in your code.

@cclauss
Copy link
Contributor

cclauss commented Oct 4, 2023

Attempting PYTHONIOENCODING=utf-8 at https://github.com/nodejs/node-gyp/pull/2862/files

Edit: No luck.

@zooba
Copy link

zooba commented Oct 4, 2023

You've got a range of apparently unrelated test failures. There doesn't seem to be any Python output in the logs, so I guess someone who knows what those tests are doing will need to take a look.

@rzhao271
Copy link
Contributor

rzhao271 commented Oct 4, 2023

See nodejs/gyp-next#201 (comment)

@cclauss
Copy link
Contributor

cclauss commented Oct 27, 2023

@brjsp It is with a heavy heart that I close your first contribution to node-gyp but I hope that this will not be your last contribution. You had the right solution and nodejs/gyp-next#201 contains the same fix. However, it was not simple as we needed to vendor in all of packaging nodejs/gyp-next#214 and then we will bring gyp-next into node-gyp. Thank you for leading the way and for your patience with our process. Python 3.12 compatibility should soon be released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants