Bug #97311 for Import-Into: New automatic module loading broke Test::Kit 2.0

Thu Jul 17 03:17:08 2014 kaoru [...] slackwise.net - Ticket created

Subject:

New automatic module loading broke Test::Kit 2.0

Hey, The new automatic module loading behaviour in import::into 1.002003 has broken the evil ways I'm using import::into in Test::Kit. http://www.cpantesters.org/cpan/report/2c4d1db0-0bfe-11e4-aeb4-166d0a370852 "Can't locate Test/Kit/Fake/MyTest/Basic/Test/More.pm in @INC (you may need to install the Test::Kit::Fake::MyTest::Basic::Test::More module)" I'm creating "fake" packages at runtime and importing things into them dynamically to make Test::Kit do its thing. The addition of the automatic loading in response to RT#96995 has broken this. I can probably fix things from the Test::Kit side by adding the "fake" packages to %INC manually... but I thought you might want to know about this and maybe make the automatic loading more optional somehow. Thanks, - Alex

Thu Jul 17 03:26:45 2014 kaoru [...] slackwise.net - Correspondence added

On Thu Jul 17 03:17:08 2014, KAORU wrote: Show quoted text

> I can probably fix things from the Test::Kit side by adding the "fake" > packages to %INC manually... but I thought you might want to know > about this and maybe make the automatic loading more optional somehow.

Here's my fix for my code: https://github.com/kaoru/Test-Kit2/commit/3f6e3ea3b2d12e05b9e0795709845ff660a6af5b I don't have a patch for import::into because I don't know your preferred solution. I suppose my recommendation would be to add import::into::with_auto_module_load() or something, and revert import::into() to its older behaviour. Anyway, up to you. Test::Kit 2.02 is on its way through the PAUSE tunnels to CPAN.

Thu Jul 17 12:06:19 2014 ether [...] cpan.org - Correspondence added

On 2014-07-17 00:17:08, KAORU wrote: Show quoted text

> Hey, > > The new automatic module loading behaviour in import::into 1.002003 > has broken the evil ways I'm using import::into in Test::Kit. > > http://www.cpantesters.org/cpan/report/2c4d1db0-0bfe-11e4-aeb4- > 166d0a370852 > > "Can't locate Test/Kit/Fake/MyTest/Basic/Test/More.pm in @INC (you may > need to install the Test::Kit::Fake::MyTest::Basic::Test::More > module)" > > I'm creating "fake" packages at runtime and importing things into them > dynamically to make Test::Kit do its thing. The addition of the > automatic loading in response to RT#96995 has broken this. > > I can probably fix things from the Test::Kit side by adding the "fake" > packages to %INC manually... but I thought you might want to know > about this and maybe make the automatic loading more optional somehow. > > Thanks, > > - Alex

We could fix this by using an interface in Module::Runtime that doesn't exist yet but we've fantasized about a bit: "load the module, and only die if it does exist and it couldn't be loaded properly; if it simply doesn't exist at all, then simply return false." -- it's on my tuit list; I'll make a note of this distribution as being another place where we could make use of it.

Thu Jul 17 12:06:20 2014 The RT System itself - Status changed from 'new' to 'open'

Thu Jul 17 12:06:24 2014 ether [...] cpan.org - Taken

Thu Jul 17 12:11:02 2014 kaoru [...] slackwise.net - Correspondence added

On Thu Jul 17 12:06:19 2014, ETHER wrote: Show quoted text

> We could fix this by using an interface in Module::Runtime that > doesn't exist yet but we've fantasized about a bit: "load the module, > and only die if it does exist and it couldn't be loaded properly; if > it simply doesn't exist at all, then simply return false." -- it's on > my tuit list; I'll make a note of this distribution as being another > place where we could make use of it.

Ooh that does sound like a good idea, good thinking. Thanks, - Alex

Thu Jul 17 12:20:33 2014 ether [...] cpan.org - Correspondence added

On 2014-07-17 00:17:08, KAORU wrote: Show quoted text

> I can probably fix things from the Test::Kit side by adding the "fake" > packages to %INC manually... but I thought you might want to know > about this and maybe make the automatic loading more optional somehow.

Oh yes, you might find this of use: https://metacpan.org/pod/me::inlined :D

Thu Jul 17 12:24:26 2014 MITHALDU [...] cpan.org - Correspondence added

As the one who's at fault for this happening: I consider this behavior to be as intended. Creating packages by emulating use/require via poking at internals should be either done completely, including updating %INC, or not at all. As such i consider the correct fix indeed to update the breaking module to update %INC properly, instead of changing I::I to possibly hide such misbehavior. Others might disagree, but so far i have not seen compelling evidence or argument to the contrary.

Thu Jul 17 12:33:54 2014 kaoru [...] slackwise.net - Correspondence added

On Thu Jul 17 12:20:33 2014, ETHER wrote: Show quoted text

> Oh yes, you might find this of use: https://metacpan.org/pod/me::inlined :D

Haha sneaky, you sure do write a lot of evil :-)

Thu Jul 17 12:40:55 2014 kaoru [...] slackwise.net - Correspondence added

On Thu Jul 17 12:24:26 2014, MITHALDU wrote: Show quoted text

> As the one who's at fault for this happening: > > I consider this behavior to be as intended. Creating packages by > emulating use/require via poking at internals should be either done > completely, including updating %INC, or not at all.

Fair enough Christian, I see your point about remaining disciplined and keeping %INC in sync with other things going on. Also it meant I discovered Module::Runtime::module_notional_filename() which was a nice find. Show quoted text

> As such i consider the correct fix indeed to update the breaking > module to update %INC properly, instead of changing I::I to possibly > hide such misbehavior.

Already done, Test::Kit 2.02 is on CPAN :-) Show quoted text

> Others might disagree, but so far i have not seen compelling evidence > or argument to the contrary.

I don't have any strong feelings either way, I leave it up to Karen and co(-maintainers.)

Thu Jul 17 12:41:33 2014 rt-cpan [...] trout.me.uk - Correspondence added

On Thu Jul 17 03:17:08 2014, KAORU wrote: Show quoted text

> Hey, > > The new automatic module loading behaviour in import::into 1.002003 > has broken the evil ways I'm using import::into in Test::Kit. > > http://www.cpantesters.org/cpan/report/2c4d1db0-0bfe-11e4-aeb4- > 166d0a370852 > > "Can't locate Test/Kit/Fake/MyTest/Basic/Test/More.pm in @INC (you may > need to install the Test::Kit::Fake::MyTest::Basic::Test::More > module)" > > I'm creating "fake" packages at runtime and importing things into them > dynamically to make Test::Kit do its thing. The addition of the > automatic loading in response to RT#96995 has broken this. > > I can probably fix things from the Test::Kit side by adding the "fake" > packages to %INC manually... but I thought you might want to know > about this and maybe make the automatic loading more optional somehow.

No, sorry, this behaviour is absolutely as intended. But I do think we owe you a better explanation as to why. The basic problem here is that because import is a UNIVERSAL method, calling RandomThing->import is a no-op, rather than an error. So, while I absolutely support your right to create on-the-fly packages for various things (and even wrote Package::Variant to make it easier :) if we allow it without %INC being set, it'll turn any typo into a silently eaten error - hence why ether's solution, though excellent for some things, is useless here. I'm going to leave this ticket open for now because most of Import::Into's lines are already an explanation of what it does and why, and we should probably explain this more clearly so people who're inconvenienced by this change understand why it was a net win, and why the less inconvenient alternatives sadly don't quite do the right thing.

Thu Jul 17 12:52:11 2014 kaoru [...] slackwise.net - Correspondence added

On Thu Jul 17 12:41:33 2014, MSTROUT wrote: Show quoted text

> No, sorry, this behaviour is absolutely as intended. But I do think we > owe you a better explanation as to why. > > The basic problem here is that because import is a UNIVERSAL method, > calling > > RandomThing->import > > is a no-op, rather than an error. > > So, while I absolutely support your right to create on-the-fly > packages for various things (and even wrote Package::Variant to make > it easier :) if we allow it without %INC being set, it'll turn any > typo into a silently eaten error - hence why ether's solution, though > excellent for some things, is useless here. > > I'm going to leave this ticket open for now because most of > Import::Into's lines are already an explanation of what it does and > why, and we should probably explain this more clearly so people who're > inconvenienced by this change understand why it was a net win, and why > the less inconvenient alternatives sadly don't quite do the right > thing.

Makes sense, thanks for the further explanation Matt. I'm convinced, keep the new behaviour for import::into and I'll keep my %INC fix in Test::Kit. - Alex

Thu Jul 17 12:52:59 2014 ether [...] cpan.org - Correspondence added

On 2014-07-17 09:41:33, MSTROUT wrote: Show quoted text

> ...and we should probably explain this more clearly so people who're > inconvenienced by this change understand why it was a net win, and why > the less inconvenient alternatives sadly don't quite do the right > thing.

Agreed, there's enough twisty passages here that a paragraph in documentation explaining it all would be helpful.