search

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 Mkbootstrap.pm strict. #368

Closed toddr closed 3 years ago

toddr commented 3 years ago

By explicitly referring to the DynaLoader package when referring to subs or vars in that space, we can make Mkbootstrap.pm strict. This was already being done in this package so we're just doing it in more places now.

Leont commented 3 years ago

This doesn't solve the problem unless the _BS files it loads also becomes strict, though its use is fairly rare nowadays

toddr commented 3 years ago

This doesn't solve the problem unless the _BS files it loads also becomes strict, though its use is fairly rare nowadays

I agree. In point of fact, outside of blead there are only 3 BS files on CPAN at all. I'm unclear if the feature is redundant and mostly unnecessary these days.

This is the entire list of files from CPAN

D/DB_File/DB_File_BS
E/Embperl/Embperl_BS
H/HTML-Embperl/Embperl_BS
p/perl/cpan/DB_File/DB_File_BS
p/perl/ext/XS-APItest/APItest_BS
craigberry commented 3 years ago

Please keep in mind that Mkbootstrap mostly runs under miniperl during the core build and thus there is no DynaLoader. The shenanigans with the DynaLoader namespace appear to be simulations of bits of DynaLoader in its absence. The patch may be ok but development on this module needs to be tested against the core build, not just against a checkout of MakeMaker.

toddr commented 3 years ago

this module needs to be tested against the core build, not just against a checkout of MakeMaker

I've been testing on blead already. I synced this here to get it into EUMM first.

toddr commented 3 years ago

I can do a pull request to blead first if you'd feel more comfortable with that @craigberry ?

craigberry commented 3 years ago

I'm not concerned about where it gets pushed first -- just offering a reminder about things that have bitten folks in the past.

toddr commented 3 years ago

After sleeping on it, I've decided to go with a more simple approach. It still smells to me that this code isn't really doing what we think it's doing, but that wasn't the problem I came here to fix. There are literally 5 modules in the world that use it so let's assume they work for now.

I am going to instead do no strict 'vars'; in the 2 places that need it and move on.