Stop inheriting from File::Spec

[mohawk2]

No description provided.

[haarg]

There's plenty of stuff in the wild that relies on EUMM inheriting from FS. I'd start by removing all of the bits that rely on it internally.

http://grep.cpan.me/?q=\%24\S%2B-%3Ecatfile+file%3AMakefile.PL

[karenetheridge]

There's plenty of stuff in the wild that relies on EUMM inheriting from FS.

right, we need to be really careful about removing this (although it is definitely a worthwhile change!) -- a grep.cpan.me scan for things like file:Makefile.PL $self->catdir (and other File::Spec interfaces) needs to be done first, and also preferably a smoke of cpan (this can be set up via a smokeme/ branch off of blead).

[mohawk2]

Yes, I found another: https://metacpan.org/source/RJBS/perl-5.20.0/cpan/Encode/Byte/Makefile.PL#L156

I'll fix this up to not remove the @ISA, but I'll add FIXME.

[mohawk2]

Back-compat preserved (and a test in t/backwards.t added), lexical dir FH used. Further comments welcome.

[mohawk2]

@karenetheridge recollects that the version of Perl to be supported is 5.6. My understanding has been that we say 5.8, but still have tests for 5.6.2, viz the travis config. @karenetheridge seeks @ribasushi's recollection of the relevant discussion to see which version was agreed on.

[ribasushi]

As for 5.6 - there are no 5.6.2 installations in the wild, but there is a ton of 5.6.1 ones, When I talk about 5.6 I generally mean 5.6.1 (as it is viable for testing, namely File::Temp appeared with it). Because in terms of compatibility 5.6.[012] are identical I always simply say 5.6 in my metadata and move on.

TL;DR: Focusing on 5.6.2 specifically is silly.

As far as the larger ticket - step back explain what you are trying to achieve with this sort of cleanup. So far it only seems to be done for the sake of... cleaning up. Which baffles me.

[Leont]

As far as the larger ticket - step back explain what you are trying to achieve with this sort of cleanup. So far it only seems to be done for the sake of... cleaning up. Which baffles me.

This PR is getting the real question totally wrong.

File::Spec is a rib taken out of MakeMaker. The catfile and catdir methods have been introduced in perl 5.001n's MakeMaker back in 1995. File::Spec was only created three years later. MakeMaker wouldn't start using File::Spec until 2001.

To me there are two questions here:

• Should we keep those methods as part of the API?
• If so, how should we implement them?

Quite frankly, IMO the answer to the first question is "YES". There is no gain in removing them, and lots of cost.

I care less about the second question. Actually it did contain wrappers around File::Spec at some point. I'm not so sure moving this from inheritance to wrappers is that much of an improvement, it surely isn't simpler.

[Leont]

I'll fix this up to not remove the @isa, but I'll add FIXME.

Right, that happened after reading the code but before me posting my reply.

The question that remains is "should we call these methods on $self or on File::Spec in our codebase. Not sure I see the point of changing it at all when not changing the inheritance.

[karenetheridge]

Not sure I see the point of changing it all when not changing the inheritance.

I think it is, if just to send the message (to all three people who will
ever read the code :)) that the @isa is purely for backwards compatibility
and no new code should be relying on it. (I'd also remove documentation for
these interfaces, or at least only mention it in small print at the end of
one document.)

On Tue, Jan 13, 2015 at 8:51 AM, Leon Timmermans [email protected]
wrote:

I'll fix this up to not remove the @isa https://github.com/ISA, but
I'll add FIXME.

Right, that happened after reading the code but before me posting my reply.

The question that remains is "should we call these methods on $self or on
File::Spec in our codebase. Not sure I see the point of changing it all
when not changing the inheritance.

—
Reply to this email directly or view it on GitHub
#196 (comment)
.

[haarg]

I'd favor changing everything to use File::Spec internally, but leaving the existing inheritance in place.

[mohawk2]

@haarg, that is now what this patch does. Arguably still worthwhile.

[craigberry]

Sorry to be joining the discussion so late and sorry to rain on the parade, but the File::Spec inheritance is actually being used by EUMM, and in 1036945 I undid the renaming of catdir and catfile in MM_Unix.pm to get things working again. I haven't (yet) looked to see if there are other places that need fixing.

The overrides in MM_VMS.pm were added in 174bc4c. The macro expansion functionality provided by the overrides was part of the original versions of catdir and catfile in EUMM and migrated more or less by accident to File::Spec when that was split off. But macro expansion has no place outside of EUMM and was eventually removed from the File::Spec versions, so it needs to be in EUMM.

Despite multiple requests to explain what this is all about, I have yet to see any rationale for it. Cryptic one-line commit messages for sweeping changes to such ancient and brittle code are really not adequate.

[Leont]

Despite multiple requests to explain what this is all about, I have yet to see any rationale for it. Cryptic one-line commit messages for sweeping changes to such ancient and brittle code are really not adequate.

I can only agree.

[Leont]

I haven't (yet) looked to see if there are other places that need fixing.

I don't really like the current in between state we're in (using $self in some places and File::Spec in others). The fact that we aren't even sure which parts can be both and which can't is not a good sign.