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

Ensures activate.bat can handle Unicode contents #2416

Closed
wants to merge 5 commits into from

Conversation

OmerFI
Copy link

@OmerFI OmerFI commented Sep 10, 2022

activate.bat is UTF-8 encoded but uses current console codepage.

This issue was encountered first in venv.
You can see the issue created for the venv module on: python/cpython#76590
And the original pull request for the venv module: python/cpython#5757

I just added some code that applies the original PR that is made for the venv module to solve this issue. The code I added is almost the same as the original commit except added @ signs.

Closes #2414

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Can we add test for this? 🤔

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Can you add a changelog entry?

@OmerFI
Copy link
Author

OmerFI commented Sep 20, 2022

I think we now don't have to change the character code page to 65001 in _get_test_lines function.

We can replace this:

        def _get_test_lines(self, activate_script):
            # for BATCH utf-8 support need change the character code page to 650001
            return ["@echo off", "", "chcp 65001 1>NUL"] + super()._get_test_lines(activate_script)

to this:

        def _get_test_lines(self, activate_script):
            return ["@echo off", ""] + super()._get_test_lines(activate_script)

@OmerFI
Copy link
Author

OmerFI commented Sep 27, 2022

I think we now don't have to change the character code page to 65001 in _get_test_lines function.

We can replace this:

        def _get_test_lines(self, activate_script):
            # for BATCH utf-8 support need change the character code page to 650001
            return ["@echo off", "", "chcp 65001 1>NUL"] + super()._get_test_lines(activate_script)

to this:

        def _get_test_lines(self, activate_script):
            return ["@echo off", ""] + super()._get_test_lines(activate_script)

Actually, this is wrong. Because a file called script.bat is created during the test and this file also should handle Unicode contents.

@OmerFI
Copy link
Author

OmerFI commented Sep 27, 2022

I think we should use utf-8 in windows not ascii to test this Unicode support, so I made these changes in the last commit:

- encoding = "ascii" if IS_WIN else sys.getfilesystemencoding()
+ encoding = sys.getfilesystemencoding()

But, when I run tests, it fails in Powershell tests. That can be because of PowerShell file does not handle Unicode contents also.

So I mark this pull request as draft until there is a solution to this problem.

@OmerFI OmerFI marked this pull request as draft September 27, 2022 12:42
@OmerFI OmerFI requested a review from gaborbernat September 28, 2022 18:23
@gaborbernat
Copy link
Contributor

But, when I run tests, it fails in Powershell tests. That can be because of PowerShell file does not handle Unicode contents also.

So I mark this pull request as draft until there is a solution to this problem.

You'll need to come up with a solution for this to land 🤔

@gaborbernat
Copy link
Contributor

Closing dut to being stalled, we can reopen if someone picks it up again.

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.

Windows PATH environment variable is not set correctly
2 participants