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

PathTools: check Config values early before chdir #23166

Merged
merged 1 commit into from
Apr 1, 2025

Conversation

haarg
Copy link
Contributor

@haarg haarg commented Mar 31, 2025

In a perl core build, the @INC paths will be relative. If you change directories before loading all of the files needed, they won't be able to be found. Config.pm loads some of its values at runtime, so if they are accessed after a chdir, it may fail.

One of the PathTools tests was relying on the fact that Config_heavy.pl would be loaded by Test::More, before it did a chdir. Newer Test::More won't do that, so the test would fail. Accessing the required Config values early will prevent this failure.

This same issue is unlikely to impact anything outside core, as it requires the perl core paths in @INC to be relative paths, which is normally not the case.

Fixes #23164

  • This set of changes does not require a perldelta entry.

In a perl core build, the @inc paths will be relative. If you change
directories before loading all of the files needed, they won't be able
to be found. Config.pm loads some of its values at runtime, so if they
are accessed after a chdir, it may fail.

One of the PathTools tests was relying on the fact that Config_heavy.pl
would be loaded by Test::More, before it did a chdir. Newer Test::More
won't do that, so the test would fail. Accessing the required Config
values early will prevent this failure.

This same issue is unlikely to impact anything outside core, as it
requires the perl core paths in @inc to be relative paths, which is
normally not the case.
@jkeenan
Copy link
Contributor

jkeenan commented Mar 31, 2025

[snip]
One of the PathTools tests was relying on the fact that Config_heavy.pl would be loaded by Test::More, before it did a chdir. Newer Test::More won't do that, so the test would fail.

Do you know when that change in Test::More's behavior occurred? (That module's git log (core) doesn't show any commit messages with 'Config' or 'heavy'.)

Accessing the required Config values early will prevent this failure.

@haarg
Copy link
Contributor Author

haarg commented Mar 31, 2025

It is the commit you identified in #23164, which will be brought in with the next version of Test::More. Test-More/test-more@1653a15

@jkeenan
Copy link
Contributor

jkeenan commented Apr 1, 2025

Locally I built a branch from this p.r., then synched the latest Test-Simple into it. All tests passed, e.g.:

$ gitcurr
gh-23166-Perl-haarg-pathtools-config-before-chdir-20250331
[perl2] 2050 $ cd t; ./perl harness -v ../dist/PathTools/t/cwd_enoent.t; cd -

ok 1 - regular getcwd result on non-existent directory
ok 2 - regular getcwd errno on non-existent directory
ok 3 - regular abs_path result on non-existent directory
ok 4 - regular abs_path errno on non-existent directory
ok 5 - perl getcwd result on non-existent directory
ok 6 - perl getcwd errno on non-existent directory
ok 7 - perl abs_path result on non-existent directory
ok 8 - perl abs_path errno on non-existent directory
ok
All tests successful.
Files=1, Tests=8,  0 wallclock secs ( 0.00 usr  0.00 sys +  0.02 cusr  0.00 csys =  0.02 CPU)
Result: PASS

So this will fix #23164. However, I won't do that synching until after this p.r. is fully approved and merged into blead.

@jkeenan
Copy link
Contributor

jkeenan commented Apr 1, 2025

Could we get one other committer to take a look at this test change in PathTools? Thanks.

@jkeenan jkeenan added the dist-PathTools issues in the dual-life blead-first PathTools distribution label Apr 1, 2025
@haarg haarg merged commit d58f204 into blead Apr 1, 2025
68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dist-PathTools issues in the dual-life blead-first PathTools distribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CPAN breaks blead: Change in Test-Simple would cause test failure dist/PathTools
3 participants