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

Fix warning from undefined thislib #390

Closed atoomic closed 3 years ago

atoomic commented 3 years ago 
===Use of uninitialized value $thislib in substitution (s///) at ../lib/ExtUtils/Liblist/Kid.pm line 72, <DATA> line 53.
Use of uninitialized value $thislib in pattern match (m//) at ../lib/ExtUtils/Liblist/Kid.pm line 99, <DATA> line 53.
Use of uninitialized value $thislib in substitution (s///) at ../lib/ExtUtils/Liblist/Kid.pm line 106, <DATA> line 53.
Use of uninitialized value $thislib in concatenation (.) or string at ../lib/ExtUtils/Liblist/Kid.pm line 114,
Leont commented 3 years ago

I'm pretty sure this fix is wrong, and would propose closing it.

Can you instead post an issue first describing how you encountered this, so that we can analyze what's really going on?

Leont commented 3 years ago

I'm pretty sure this fix is wrong, and would propose closing it.

Can you instead post an issue first describing how you encountered this, so that we can analyze what's really going on?

After some experimenting I discovered that if $potential_libs equals " ", quotewords will return the list ('', undef). I'm guessing that's how this is triggered. This issue was introduced in d21fe269f.

Possibly we should be using shellwords instead, but we should check to be sure if that doesn't break anything (this is LibList, the odds are non-trivial). @mohawk2: is there a specific reason you used quotewords instead of shellwords?

atoomic commented 3 years ago

let's close it, this was a quick n dirty suggestion to fix the warning from blead in 5.33.8

Leont commented 3 years ago

let's close it, this was a quick n dirty suggestion to fix the warning from blead in 5.33.8

MakeMaker is not the kind of codebase where quick and dirty suggestions are helpful, it already contains more dirt than it can carry. Next time please file a bug report instead.

wchristian commented 3 years ago

As someone who worked on this and spent hours trawling and tracing through this code with the realtime debugger, and added tests for everything:

Please always make sure that any change is done with with plenty time invested so you know exactly what you're doing and includes copious amounts of tests verifying circumstances before and after your change.

This is not a place where anything quick, nevermind dirty, can be done.

atoomic commented 3 years ago

reported as #391 and fixed as #392