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

Fix STDIN input encoding for Windows #447

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

hakonhagland
Copy link
Contributor

@hakonhagland
Copy link
Contributor Author

Not sure why this test fails:

Use of uninitialized value $codepage in concatenation (.) or string at D:\a\ExtUtils-MakeMaker\ExtUtils-MakeMaker\blib\lib/ExtUtils/MakeMaker.pm line 229.
Unknown encoding 'cp' at D:\a\ExtUtils-MakeMaker\ExtUtils-MakeMaker\blib\lib/ExtUtils/MakeMaker.pm line 229.
# Looks like you planned 11 tests but ran 9.
# Looks like your test exited with 255 just after 9.
t/prompt.t ................ 

The test does not fail on my laptop..

@jkeenan
Copy link
Contributor

jkeenan commented Sep 19, 2023

Not sure why this test fails:

Use of uninitialized value $codepage in concatenation (.) or string at D:\a\ExtUtils-MakeMaker\ExtUtils-MakeMaker\blib\lib/ExtUtils/MakeMaker.pm line 229.
Unknown encoding 'cp' at D:\a\ExtUtils-MakeMaker\ExtUtils-MakeMaker\blib\lib/ExtUtils/MakeMaker.pm line 229.
# Looks like you planned 11 tests but ran 9.
# Looks like your test exited with 255 just after 9.
t/prompt.t ................ 

The test does not fail on my laptop..

Perhaps in your p.r. at line 223 the return value was an empty string. That would be a defined value, but then at 229 the first argument to decode would only be cp.

To fix a test failure, I am checking if it is caused by
Win32::GetConsoleCP()
returning an empty string
@hakonhagland
Copy link
Contributor Author

Perhaps in your p.r. at line 223 the return value was an empty string

@jkeenan Added a test for empty string, see last commit

@Leont
Copy link
Member

Leont commented Sep 19, 2023

I'm not sure if we can use Encode here like that; MakeMaker is part of perl's bootstrap which means it's used before Encode is built. ExtUtils::MakeMaker::Locale is some prior art in this area.

@hakonhagland
Copy link
Contributor Author

I'm not sure if we can use Encode here like that

Then as an alternative, would it be possible to apply this patch to e.g. IO::Prompt::Tiny instead and patch consumers like CPAN::Shell to use IO::Prompt::Tiny::prompt as a replacement for ExtUtils::MakeMaker::prompt ?

@Leont
Copy link
Member

Leont commented Sep 19, 2023

Then as an alternative, would it be possible to apply this patch to e.g. IO::Prompt::Tiny instead and patch consumers like CPAN::Shell to use IO::Prompt::Tiny::prompt as a replacement for ExtUtils::MakeMaker::prompt ?

No. CPAN::Shell is core so can't use any module outside of core.

And I didn't you can't use Encode at all, I'm just saying it should handle its absence gracefully.

@hakonhagland
Copy link
Contributor Author

ExtUtils::MakeMaker::Locale is some prior art in this area.

Yes, nice one! For reference here is a link to the PR from 2014: #125

@hakonhagland
Copy link
Contributor Author

hakonhagland commented Sep 19, 2023

MakeMaker is part of perl's bootstrap which means it's used before Encode is built.

@Leont But why then can ExtUtils::MakeMaker::Locale use Encode itself? See line 15:

@Leont
Copy link
Member

Leont commented Sep 19, 2023

@Leont But why then can ExtUtils::MakeMaker::Locale use Encode itself? See line 15:

Because this

Use bundled local module to determine windows codepage instead.
@hakonhagland
Copy link
Contributor Author

I'm not sure if we can use Encode here like that

Trying with ExtUtils::MakeMaker::Locale instead, see latest commit. Also added a test case.

Forgot to increase test number in prompt.t
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.

registration failed: Internal Server Error at Metabase/Client/Simple.pm line 195.
3 participants