Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filename normalization in trace object means the filename does not match perl internals #16

Open
haarg opened this issue Nov 29, 2017 · 3 comments

Comments

@haarg
Copy link
Contributor

haarg commented Nov 29, 2017

The filename in stack traces is normalized with File::Spec->canonpath. On unix systems, this will usually not change the reported file name. On Win32, it will usually change the directory separators. This makes the filename not match what perl thinks it is for the files, so it won't match things like __FILE__, %INC keys, internal error messages, or other uses of caller, including Carp. The normalization doesn't seem particularly useful to begin with in this case, so I think it may be better to remove it. There could be some backwards compatibility issues with changing it though.

@autarch
Copy link
Member

autarch commented Nov 29, 2017

Yeah, this was clearly a bad idea. According to git, I did it in 2005 but I didn't provide a reason in the commit message. Bad me.

@autarch autarch changed the title filename normalization make filename not match perl internals Filename normalization in trace object means the filename does not match perl internals Dec 6, 2017
@lancew
Copy link

lancew commented Mar 21, 2018

Looking at this one for the CPAN PRC,
removing $self->{filename} = File::Spec->canonpath( $self->{filename} );
This seems to work but causes one test to fail:

not ok 35 - filename is canonicalized
#   Failed test 'filename is canonicalized'
#   at t/01-basic.t line 338.
#          got: 'foo/bar///baz.pm'
#     expected: 'foo/bar/baz.pm'

This test is solely for Linux (else skipped) so is it ok to remove it?

@autarch
Copy link
Member

autarch commented Mar 21, 2018

My one concern is that this is a backwards incompatible change, which could break who knows what out there.

I think I'll have to do a dev release at the very least.

As for the test, I think it can be removed.

lancew added a commit to cv-library/Devel-StackTrace that referenced this issue Mar 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants