Perl-Toolchain-Gang / ExtUtils-MakeMaker

Perl module to make Makefiles and build modules (what backs Makefile.PL)
https://metacpan.org/release/ExtUtils-MakeMaker
64 stars 77 forks source link

Fix STDIN input encoding for Windows #447

Open hakonhagland opened 1 year ago

hakonhagland commented 1 year ago

See https://github.com/cpan-testers/Test-Reporter-Transport-Metabase/issues/3 and https://stackoverflow.com/q/77123411/2173773. I believe this might fix the issue.

hakonhagland commented 1 year ago

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 commented 1 year ago

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.

hakonhagland commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Yes, nice one! For reference here is a link to the PR from 2014: https://github.com/Perl-Toolchain-Gang/ExtUtils-MakeMaker/pull/125

hakonhagland commented 1 year ago

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:

https://github.com/Perl-Toolchain-Gang/ExtUtils-MakeMaker/blob/694b7c78ab4d505d86a805db9dc0d441f08aabc2/lib/ExtUtils/MakeMaker/Locale.pm#L15

Leont commented 1 year ago

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

Because this

hakonhagland commented 1 year ago

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.