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 76 forks source link

Make windows specific code for NoXS compile under strict #366

Closed toddr closed 3 years ago

toddr commented 3 years ago

Fixes #365

Grinnz commented 3 years ago

Looks correct (and much cleaner) to me

haarg commented 3 years ago
delete @Win32::{ grep { $_ ne "GetCwd" && $_ ne "SetChildShowWindow" } keys %Win32:: };
toddr commented 3 years ago

delete @Win32::{ grep { $ ne "GetCwd" && $ ne "SetChildShowWindow" } keys %Win32:: };

Yep. saw this late. I decided the for loop would be more clear to people?

wchristian commented 3 years ago

Fwiw, i didn't mean to start a review. I am not sure how to use that interface. So i now clicked around on a few things and hope that made things better. :)

haarg commented 3 years ago

Technically this looks fine. It might be worth explaining why the deletion is being done. Possibly something like:

# SetChildShowWindow and GetCwd are provided by perl core in modern perls, so we
# can use them in miniperl. on older perls, we can load them from Win32 so the
# test can proceed as normal.
toddr commented 3 years ago

SetChildShowWindow and GetCwd are provided by perl core in modern perls, so we can use them in miniperl. on older perls, we can load them from Win32 so the test can proceed as normal.

Part of my problem here was divining what @bulk88 's original intent was here. Is this for sure what's being done?

haarg commented 3 years ago

Yes.

The test is to verify that EUMM works properly in miniperl, which is before any XS modules are available. SetChildShowWindow and GetCwd are provided directly by perl on modern perls, not by the Win32 module. So they are allowed to be used by EUMM under miniperl. However, older perls don't provide them. Loading the Win32 module, then removing all of the other symbols simulates the situation on newer perls, which allows the miniperl test to continue normally. The miniperl behavior is only needed at perl build time, so simulating a modern perl is acceptable for testing. Current EUMM won't ever be used for building older perls.

toddr commented 3 years ago

comment updated.